Skip to content

fix(lumina): break vector search score ties deterministically - #725

Open
jackylee-ch wants to merge 1 commit into
apache:mainfrom
jackylee-ch:fix/lumina-deterministic-tie-break
Open

fix(lumina): break vector search score ties deterministically#725
jackylee-ch wants to merge 1 commit into
apache:mainfrom
jackylee-ch:fix/lumina-deterministic-tie-break

Conversation

@jackylee-ch

Copy link
Copy Markdown
Contributor

collect_results in the Lumina reader kept the top-k by score alone, so rows
sharing a score were retained on a first-come basis and the arrival order comes
from the native searcher. The same query against the same index could therefore
return a different row set, and the eviction check used a bare score > peek.score rather than a total order.

This is the one search backend the invariant established by #613 and #614 did
not reach. vector_search::ScoredRow already breaks ties on the row id and
routes eviction through is_stronger_than; full_text::top_k carries the same
rule with a comment stating the order must be deterministic regardless of input
or shard order.

Fix: mirror the sibling comparator — total_cmp on the score, then the row
id — and evict through it instead of >. Among equal scores the smallest row id
now wins, in both the single-query and the batch path, which slice the same
helper.

Verified at the unit level: the three new tests feed tied scores in permuted
orders and fail on current main. Reproducing the drift end to end needs
LUMINA_LIB_PATH, which CI does not provide, so no run-time evidence is claimed
here — the tests cover the selection logic directly.

min_heap.push(ScoredRow { row_id, score });
}
min_heap.push(entry);
} else if min_heap

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both production callers pass exactly effective_k labels and the same value as top_k, so labels.len() <= top_k and this replacement branch cannot execute. The new tests trigger it only by passing more candidates than production can return. If ties are discarded at the native top-k cutoff, those rows never reach this comparator. Please enforce the tie-break before that cutoff (or return more than k candidates) and test through the real call contract.

impl ScoredRow {
fn is_stronger_than(&self, other: &Self) -> bool {
self.score
.total_cmp(&other.score)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Do not promote NaN scores above finite candidates

f32::total_cmp ranks a positive NaN above every finite score, so this changes more than the tie-break. With top_k = 1 and native results [(row 8, 0.5), (row 7, NaN)], the current code ignores the later NaN because NaN > 0.5 is false, while this comparator evicts row 8 and returns the NaN-scored row. A non-finite stored vector can produce such a distance; the PK-vector metric code uses java_float_compare specifically to keep all NaNs worst. Please skip/reject non-finite scores or use NaN-worst comparison semantics here, and change the regression test so a finite candidate wins regardless of arrival order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants