Skip to content

fix(fts): deterministic top-k tiebreak for tied scores - #7846

Open
sbrunk wants to merge 12 commits into
lance-format:mainfrom
sbrunk:fts-deterministic-topk-tiebreak
Open

fix(fts): deterministic top-k tiebreak for tied scores#7846
sbrunk wants to merge 12 commits into
lance-format:mainfrom
sbrunk:fts-deterministic-topk-tiebreak

Conversation

@sbrunk

@sbrunk sbrunk commented Jul 19, 2026

Copy link
Copy Markdown

Problem

Lance's BM25 top-k resolves equal scores by encounter and pruning order, which varies with k and with the order partitions finish. So top-k1 is not an ordered prefix of top-k2, and paginating a broad tied-score term (many docs sharing tf and doc length, so identical BM25) duplicates and skips rows across from/size pages.

Lucene avoids this with a deterministic secondary sort (score DESC, docId ASC). This PR brings the same guarantee to Lance's plain FTS path: supports_compound_scorer returns false for a top-level Match/Phrase, so the most common query runs MatchQueryExec/PhraseQueryExecbm25_searchwand::TopKCollector, which is score-only. The collector mirrors the compound scorer's shape so the two cannot drift.

Change

  • ScoredDoc::cmp breaks score ties by ascending row id, then ascending doc_index for list-element granularity.
  • The per-partition collector compares on (score, order_key) and retains rather than ranks whatever compares equal. The order key is the resolved row address, which is unique per document, so at row granularity the tie band stays empty and the hot path is one pop and one push. In deferred mode equal means a score tie; for list elements it means same row and same score, which is exactly the ambiguity the WAND walk cannot see.
  • The WAND threshold sits one ULP below the k-th score (admit_ties_floor), so score-only prune tests keep, not drop, docs tied at the k-th. The pruning kernels are untouched.
  • Both tie bands are bounded at limit + 128. A deferred overflow reports fts_score_floor_overflows and retries the affected partitions in parallel with an address projection attached; an element band that overflows reports fts_element_band_truncations and degrades to ranking on the posting DocId.
  • The cross-partition merge compacts past the bound, keeping the exact top-k by (score, row_id, doc_index) and restarting the band.
  • The cross-segment merge (search_segments) and the indexed/flat union sort both carry the full key, with _doc_index appended when the schema is element-granular.
  • Element coordinate columns are cached per partition, so repeated resolution during compaction is a slice read rather than an O(P²) scatter read.

Row ids

ScoredDoc.row_id in the collector holds a document key: a row address for legacy indexes, a dense per-partition DocId for modern ones. Address order matches DocId order only for un-remapped partitions, so the tiebreak keys on the resolved row address.

Correctness within a dataset version does not depend on stable row ids; the key only needs to be unique and comparable, which holds for both legacy row addresses and stable row ids. Stable row ids additionally keep the order stable across compaction. This mirrors Lucene, whose docId tiebreak is deterministic within a searcher version but not stable across segment merges.

Legacy path

push_scored_key evicts on the full key, and drops NaN scores rather than admitting them. OrderedFloat uses total_cmp, so an admitted NaN would rank first in an under-filled heap; the WAND collector drops NaN once full, so both layers now agree. NaN is unreachable from stock BM25 and only arises from a caller-supplied scorer.

Tests

  • Stable prefix (top-k1 is an ordered prefix of top-k2) across partitions, across segments committed in reverse order, through a prefilter, with unindexed rows in the union, and repeated over 12 striped partitions.
  • Element-document ties resolved by doc_index, end to end and through the indexed/flat union.
  • Bounds: the collector band, the element band, and the cross-partition merge band each assert the bound holds and that the correct documents survive.
  • Overflow retry, including every partition overflowing at once, asserting the metric and exact results.
  • Collector units for full-key eviction, slot reuse, NaN rejection, and admit_ties_floor at zero, negative, and NaN.
  • Legacy push_scored_key tiebreak and NaN rejection.
  • cargo test -p lance-index --lib 974 passed, -p lance --lib 2700 passed.

Performance

The change is a no-op on the non-tied path by construction: row-granularity order keys are unique, so the tie band stays empty and the collector does one pop and one push as before. The one thing that needed measuring is admit_ties_floor, which lowers the WAND pruning threshold by one ULP.

cargo bench -p lance-index --bench inverted, 1M docs, criterion (10 s measurement, 10 samples), Apple M3 Max. Both bench binaries built up front and run interleaved with alternating order across 10 rounds; round 0 discarded as warm-up, rounds 1-9 measured, each with its own CRITERION_HOME.

case before (median) after (median) paired mean Δ
invert_search(1000000) 20.42 ms 20.89 ms -0.02%
invert_phrase_search(1000000) 6.39 ms 6.26 ms -1.89%
invert_indexing(1000000) 9.66 s 9.54 s +0.40%

Run-to-run spread is 5-8% for search and 13-15% for indexing, so read the paired per-round mean rather than the medians.

  • Search is unchanged. Paired mean -0.02%. Per-round deltas swing ±4% but track run order (-1.70% when before ran first, +2.07% when after did), which is thermal. The one-ULP threshold change costs no measurable pruning power: on a Zipf corpus a block max rarely bit-equals the running k-th score.
  • Phrase search is ~2% faster, consistent in sign (8 of 9 rounds, both order splits agree), but smaller than the absolute spread and marginal under multiple-comparison correction. Treat as "not a regression".
  • Indexing is unchanged. The ScoredDoc::cmp tiebreak adds no measurable build cost; the +0.40% is one outlier round (sign test p=0.51).

@github-actions github-actions Bot added A-index Vector index, linalg, tokenizer bug Something isn't working labels Jul 19, 2026
@coderabbitai

coderabbitai Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The inverted-index search paths now preserve deterministic top-k results for equal BM25 scores. Boundary ties are retained during WAND and BM25 collection, deferred row IDs are resolved before final sorting, and results use score-descending then row_id-ascending ordering.

Changes

Deterministic top-k ranking

Layer / File(s) Summary
Scored ordering and BM25 merge
rust/lance-index/src/scalar/inverted/builder.rs, rust/lance-index/src/scalar/inverted/index.rs
Equal scores use deterministic row_id ordering; BM25 merging retains boundary ties, resolves deferred row IDs, and tests stable results across partitions, filters, and varying limits.
Deferred boundary-tie collection
rust/lance-index/src/scalar/inverted/wand.rs
TopKCollector retains k-th-score ties when row IDs are deferred and includes them in final candidates.
WAND threshold admission and collector wiring
rust/lance-index/src/scalar/inverted/wand.rs
WAND thresholds are lowered by one ULP to admit ties, and search modes configure deferred tie-breaking based on row ID availability.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Query as WAND search
  participant Collector as TopKCollector
  participant Merge as BM25 merge
  participant Results as Final candidates
  Query->>Collector: insert scored documents
  Collector->>Collector: retain k-th-score boundary ties
  Collector->>Merge: materialize heap and deferred ties
  Merge->>Results: resolve row_ids and sort deterministically
  Results->>Query: return top-k candidates
Loading

Possibly related PRs

Suggested reviewers: bubblecal, xuanwo, westonpace

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: deterministic FTS top-k tie-breaking by score and row_id.
Description check ✅ Passed The description is directly related and accurately describes the BM25 top-k tie-breaking fix and tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)
rust/lance-index/src/scalar/inverted/index.rs-11628-11662 (1)

11628-11662: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use a masked non-flat predicate for this filtered FTS test.

bm25_search passes the prefilter’s OR result back into docs_for_wand(...), and the relevant existing test shows an OR with an allow-list can still return !has_row_ids() before the outer deferred CandidateAddr::Pending(doc_id) resolver fixes candidates. Because test_fts_topk_tied_scores_stable_prefix_filtered also uses Operator::Or, its “forces the real row_id / full-key eviction path” claim is not guaranteed by the current setup. Use a masked non-flat predicate such as Operator::And with an all-rows allow-list so this test matches test_fts_topk_tied_scores_stable_prefix_unsorted_row_ids without relying on path-coverage assumptions.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-index/src/scalar/inverted/index.rs` around lines 11628 - 11662,
Update test_fts_topk_tied_scores_stable_prefix_filtered to use a masked non-flat
prefilter, such as Operator::And combined with the all-rows AllowListFilter,
instead of the current Operator::Or setup. Match the predicate construction used
by test_fts_topk_tied_scores_stable_prefix_unsorted_row_ids, while preserving
the existing top3/top5 assertions and stable row_id ordering.
🧹 Nitpick comments (4)
rust/lance-index/src/scalar/inverted/index.rs (3)

11539-11560: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add #[cfg_attr(coverage, coverage(off))] to this test-utility impl.

The other test doubles in this file (DocsRowIdReadCounter, DocsRowIdCountingReader, DocsRowIdCountingStore) carry it. As per coding guidelines: "disable coverage for test utilities with #[cfg_attr(coverage, coverage(off))]".

♻️ Proposed change
+    #[cfg_attr(coverage, coverage(off))]
     #[async_trait::async_trait]
     impl PreFilter for AllowListFilter {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-index/src/scalar/inverted/index.rs` around lines 11539 - 11560,
Add #[cfg_attr(coverage, coverage(off))] to the AllowListFilter PreFilter
implementation, matching the existing test utilities DocsRowIdReadCounter,
DocsRowIdCountingReader, and DocsRowIdCountingStore. Do not alter the
implementation methods or behavior.

Source: Coding guidelines


1232-1232: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Row-id resolution is IO-bound; consider self.store.io_parallelism() for the fan-out.

resolve_row_ids reads the docs file's ROW_ID column, and every other IO fan-out in this file (aggregate_corpus_stats, df_for_term, partition loading) is bounded by self.store.io_parallelism(). Using the compute-intensive CPU count here is inconsistent and does not track store IO capacity.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-index/src/scalar/inverted/index.rs` at line 1232, Update the
fan-out in resolve_row_ids to use self.store.io_parallelism() instead of
get_num_compute_intensive_cpus(). Keep the existing buffer_unordered behavior
and ensure the concurrency limit follows the store’s IO capacity, consistent
with the other IO-bound paths.

938-962: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Replace the bare .unwrap()s on the heap peeks/pops with .expect("reason") (or return an error).

Lines 942, 952, 954 panic if the invariant ever breaks. The analogous collector in wand.rs (TopKCollector::take_worst) returns Error::internal for exactly this case, so the two merge paths are inconsistent. As per coding guidelines: "Never use .unwrap(), .expect(), panic!(), or assert!() in library code for fallible operations" and "Avoid bare .unwrap(); ... If unavoidable, use .expect(\"reason\")."

♻️ Minimal fix using `expect`
-            let kth = candidates.peek().unwrap().0.score;
+            let kth = candidates
+                .peek()
+                .expect("heap is full, so it holds at least one candidate")
+                .0
+                .score;
             match candidate.score.cmp(&kth) {
                 // Below the k-th score: never competitive.
                 std::cmp::Ordering::Less => {}
                 // Tied at the k-th score: a potential winner once row_ids are
                 // resolved, so keep it in the boundary band.
                 std::cmp::Ordering::Equal => overflow.push(candidate),
                 // Strictly better on score: it enters the heap. Whether the
                 // displaced candidate stays a boundary tie depends on the new k-th.
                 std::cmp::Ordering::Greater => {
-                    let Reverse(displaced) = candidates.pop().unwrap();
+                    let Reverse(displaced) =
+                        candidates.pop().expect("heap is full, so pop yields the k-th");
                     candidates.push(Reverse(candidate));
-                    let new_kth = candidates.peek().unwrap().0.score;
+                    let new_kth = candidates
+                        .peek()
+                        .expect("a candidate was just pushed")
+                        .0
+                        .score;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-index/src/scalar/inverted/index.rs` around lines 938 - 962,
Replace the bare unwrap calls in the candidate heap handling around
candidates.peek() and candidates.pop() with descriptive expect messages, or
propagate an appropriate internal error consistent with wand.rs
TopKCollector::take_worst. Preserve the existing heap-selection and overflow
behavior while making invariant failures explicit rather than using bare
unwraps.

Source: Coding guidelines

rust/lance-index/src/scalar/inverted/wand.rs (1)

4413-4487: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Good collector coverage; add a direct unit test for admit_ties_floor edge inputs.

The tie-admission scheme rests on admit_ties_floor, but its documented edge behavior (0.0, negative, and NaN inputs relative to the threshold > 0.0 guards) is only covered indirectly. A three-line rstest locks the contract in place. As per coding guidelines: "Every bugfix and feature must have corresponding tests" and "Use rstest for Rust parameterized tests, use readable #[case::{name}(...)] names".

💚 Proposed test
#[rstest]
#[case::positive(1.0)]
#[case::small(f32::MIN_POSITIVE)]
fn test_admit_ties_floor_admits_kth_score_ties(#[case] kth: f32) {
    let floor = admit_ties_floor(kth);
    assert!(floor < kth, "floor {floor} must sit below the k-th score {kth}");
    assert!(kth > floor, "a doc tied at the k-th score must pass `score > threshold`");
}

#[rstest]
#[case::zero(0.0)]
#[case::negative(-1.0)]
fn test_admit_ties_floor_keeps_non_positive_read_as_no_threshold(#[case] kth: f32) {
    assert!(!(admit_ties_floor(kth) > 0.0));
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-index/src/scalar/inverted/wand.rs` around lines 4413 - 4487, Add
direct rstest coverage for the admit_ties_floor function, using readable named
cases for positive values (including f32::MIN_POSITIVE), zero, and negative
input. Assert positive inputs produce a floor below the k-th score, and zero or
negative inputs do not produce a positive threshold; place the tests alongside
the existing TopKCollector tests.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@rust/lance-index/src/scalar/inverted/index.rs`:
- Around line 1226-1239: Update the deferred row-ID resolution flow around
resolve_row_ids and the subsequent entries.into_iter().zip(row_ids) loop to
validate that every requested doc_id produced a row ID before assigning results.
Detect any short resolution instead of allowing zip to truncate and preserve the
placeholder, then return an appropriate error consistent with the existing
resolve_deferred_candidates guard.

---

Other comments:
In `@rust/lance-index/src/scalar/inverted/index.rs`:
- Around line 11628-11662: Update
test_fts_topk_tied_scores_stable_prefix_filtered to use a masked non-flat
prefilter, such as Operator::And combined with the all-rows AllowListFilter,
instead of the current Operator::Or setup. Match the predicate construction used
by test_fts_topk_tied_scores_stable_prefix_unsorted_row_ids, while preserving
the existing top3/top5 assertions and stable row_id ordering.

---

Nitpick comments:
In `@rust/lance-index/src/scalar/inverted/index.rs`:
- Around line 11539-11560: Add #[cfg_attr(coverage, coverage(off))] to the
AllowListFilter PreFilter implementation, matching the existing test utilities
DocsRowIdReadCounter, DocsRowIdCountingReader, and DocsRowIdCountingStore. Do
not alter the implementation methods or behavior.
- Line 1232: Update the fan-out in resolve_row_ids to use
self.store.io_parallelism() instead of get_num_compute_intensive_cpus(). Keep
the existing buffer_unordered behavior and ensure the concurrency limit follows
the store’s IO capacity, consistent with the other IO-bound paths.
- Around line 938-962: Replace the bare unwrap calls in the candidate heap
handling around candidates.peek() and candidates.pop() with descriptive expect
messages, or propagate an appropriate internal error consistent with wand.rs
TopKCollector::take_worst. Preserve the existing heap-selection and overflow
behavior while making invariant failures explicit rather than using bare
unwraps.

In `@rust/lance-index/src/scalar/inverted/wand.rs`:
- Around line 4413-4487: Add direct rstest coverage for the admit_ties_floor
function, using readable named cases for positive values (including
f32::MIN_POSITIVE), zero, and negative input. Assert positive inputs produce a
floor below the k-th score, and zero or negative inputs do not produce a
positive threshold; place the tests alongside the existing TopKCollector tests.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: 07a492b0-2bd6-43ee-9af1-781768a51473

📥 Commits

Reviewing files that changed from the base of the PR and between 6badd5f and a7f155e.

📒 Files selected for processing (3)
  • rust/lance-index/src/scalar/inverted/builder.rs
  • rust/lance-index/src/scalar/inverted/index.rs
  • rust/lance-index/src/scalar/inverted/wand.rs

Comment thread rust/lance-index/src/scalar/inverted/index.rs Outdated

@coderabbitai coderabbitai Bot left a comment

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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
rust/lance-index/src/scalar/inverted/wand.rs (1)

160-195: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

NaN-score guard is unreachable while the heap is still filling up.

The NaN check at lines 191-195 only executes once the heap is full (self.heap.len() == self.limit). The earlier "heap not yet full" fast path (lines 178-184) pushes doc unconditionally, before the NaN check runs. So a NaN-scored candidate is accepted into the top-k while the collector is still filling, but rejected once it reaches capacity — inconsistent with the stated intent ("A NaN custom-scorer score is never competitive; drop it"). This can let an uncompetitive/invalid document occupy a top-k slot (and, in deferred mode, be treated as tied at whatever the eventual k-th score becomes).

🐛 Proposed fix: move the NaN check before the not-yet-full fast path
     ) -> Result<bool> {
         if self.limit == 0 {
             return Ok(false);
         }
+        // A NaN custom-scorer score is never competitive; drop it before it can enter the
+        // heap while still filling (the full-heap branch below already excludes it).
+        if doc.score.0.is_nan() {
+            return Ok(false);
+        }
         if self.heap.len() > self.limit {
             return Err(Error::internal(format!(
                 "FTS top-k heap length {} exceeds limit {}",
                 self.heap.len(),
                 self.limit
             )));
         }
 
         // Heap not yet full: everything is competitive.
         if self.heap.len() < self.limit {
             let slot = self.frequency_slots.push(pairs)?;
             self.heap
                 .push(Reverse((doc, doc_length, posting_doc_id, slot)));
             return Ok(true);
         }
 
         let Some(worst) = self.heap.peek().map(|entry| entry.0.0.clone()) else {
             return Err(Error::internal(
                 "FTS top-k heap is empty while its nonzero limit is reached",
             ));
         };
-        // A NaN custom-scorer score is never competitive; drop it (also keeps NaN out of the
-        // `total_cmp` key, matching the pre-tiebreak collector).
-        if doc.score.0.is_nan() {
-            return Ok(false);
-        }

None of the new tests (test_top_k_collector_deferred_tiebreak_retains_boundary_ties, test_top_k_collector_full_key_eviction_by_row_id, test_top_k_collector_reuses_frequency_slots) exercise a NaN insert while the heap is below limit, so this regressed silently. Worth adding a regression test alongside the fix.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-index/src/scalar/inverted/wand.rs` around lines 160 - 195, Move
the NaN-score guard in insert before the heap-not-full fast path so
doc.score.0.is_nan() always returns Ok(false), including while the heap is
filling. Preserve the existing limit and capacity checks, and add a regression
test covering insertion of a NaN-scored document when the heap length is below
limit.
♻️ Duplicate comments (1)
rust/lance-index/src/scalar/inverted/index.rs (1)

1234-1257: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

debug_assert_eq! still lets a short resolve_row_ids return silently corrupt results in release builds.

This is the same risk flagged in a prior review (resolve_row_ids returning fewer row_ids than requested leaves the placeholder row_id = 0 set at line 1230, via the zip truncating silently). The fix replaces the earlier no-check code with debug_assert_eq!, but debug_assert! compiles out in release builds, so in production a length mismatch still produces a wrong-row (0) result with no error — exactly the scenario the prior review asked to guard against with an explicit Err.

As per coding guidelines: "Do not silently guard against impossible conditions; use debug_assert!, return an explicit error, or remove the check" and "Prefer debug_assert! over assert! for non-safety invariants; reserve assert! for conditions preventing data corruption" — a silently-wrong row_id returned to the caller is exactly a data-corruption outcome, so this invariant belongs behind an explicit Result check (or assert!), not debug_assert!.

🛡️ Proposed fix
             for (entries, row_ids) in batches {
-                // `resolve_row_ids` maps one row_id per requested doc_id, so the lengths always match;
-                // the assert documents that invariant (a short return would otherwise leave placeholder
-                // row 0 via the `zip` below).
-                debug_assert_eq!(entries.len(), row_ids.len());
+                if entries.len() != row_ids.len() {
+                    return Err(Error::internal(format!(
+                        "resolve_row_ids returned {} row_ids for {} deferred doc_ids",
+                        row_ids.len(),
+                        entries.len()
+                    )));
+                }
                 for ((pos, _), row_id) in entries.into_iter().zip(row_ids) {
                     resolved[pos].1 = row_id;
                 }
             }

As per coding guidelines: "Do not silently guard against impossible conditions; use debug_assert!, return an explicit error, or remove the check." Based on a past review comment on this exact code (resolve_row_ids truncation via zip), which proposed the same explicit-error fix that was not fully applied here.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance-index/src/scalar/inverted/index.rs` around lines 1234 - 1257,
Replace the `debug_assert_eq!` in the deferred row-resolution loop with an
explicit error check that returns an error when `entries.len()` and
`row_ids.len()` differ, before the `zip` iteration. Preserve normal resolution
for matching lengths and ensure release builds cannot leave placeholder row IDs
after `resolve_row_ids` truncates the mismatch.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@rust/lance-index/src/scalar/inverted/wand.rs`:
- Around line 160-195: Move the NaN-score guard in insert before the
heap-not-full fast path so doc.score.0.is_nan() always returns Ok(false),
including while the heap is filling. Preserve the existing limit and capacity
checks, and add a regression test covering insertion of a NaN-scored document
when the heap length is below limit.

---

Duplicate comments:
In `@rust/lance-index/src/scalar/inverted/index.rs`:
- Around line 1234-1257: Replace the `debug_assert_eq!` in the deferred
row-resolution loop with an explicit error check that returns an error when
`entries.len()` and `row_ids.len()` differ, before the `zip` iteration. Preserve
normal resolution for matching lengths and ensure release builds cannot leave
placeholder row IDs after `resolve_row_ids` truncates the mismatch.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: 7f896b81-bab5-4b14-be10-8e898b1ce3d4

📥 Commits

Reviewing files that changed from the base of the PR and between a7f155e and 8fa1753.

📒 Files selected for processing (2)
  • rust/lance-index/src/scalar/inverted/index.rs
  • rust/lance-index/src/scalar/inverted/wand.rs

@sbrunk

sbrunk commented Jul 27, 2026

Copy link
Copy Markdown
Author

@LuQQiu this touches your optimization in #7897 a bit but should not cause regressions in general (see the performance section).

@sbrunk
sbrunk force-pushed the fts-deterministic-topk-tiebreak branch 2 times, most recently from 83d8f99 to 3ce7d38 Compare August 5, 2026 15:11
@sbrunk sbrunk changed the title fix(fts): deterministic top-k tiebreak (score DESC, row_id ASC) fix(fts): deterministic top-k tiebreak for tied scores Aug 5, 2026
@sbrunk
sbrunk force-pushed the fts-deterministic-topk-tiebreak branch from 3ce7d38 to 03cbcb6 Compare August 6, 2026 15:22
sbrunk added 12 commits August 11, 2026 07:36
BM25 top-k resolved equal scores by encounter and pruning order, which varies
with k and with partition completion order. top-k1 was not a prefix of top-k2,
so paginating a tied-score query duplicated and skipped rows.

Give the plain (non-compound) FTS top-k the same total order the compound
scorer already uses, keyed on the resolved row address:

- ScoredDoc::cmp breaks score ties by ascending row_id, then doc_index, and is
  the order the final sort applies.
- The WAND collector evicts on the full key when the walk knows row addresses:
  legacy postings, and modern partitions whose address projection is resident.
- Modern partitions that resolve addresses only after the walk cannot order a
  tie, so the collector retains the whole k-th-score band beside its heap,
  bounded at limit + 128 like the compound collector. A wider band reports an
  overflow and the partition is rescored against a loaded address projection.
- The WAND threshold sits one ULP below the k-th score, so the score-only prune
  kernels keep, rather than drop, documents tied at the k-th score, including a
  slower partition's ties behind the shared cross-partition floor. The kernels
  themselves are unchanged.
- The cross-partition merge keeps the same band and re-selects by
  (score DESC, row_id ASC) once addresses are resolved.

Non-tied queries keep full pruning power and an unchanged working set: the band
stays empty and eviction is still one pop and one push.
The index-level top-k is now ordered by (score DESC, row_id ASC), but two merges
above it still ranked on score alone and undid that for a tied query.

- search_segments merges the per-segment results of a match or phrase query with
  a score-only heap. Segments complete in whatever order buffer_unordered
  yields, so a tie was resolved by segment timing. Evict on the full key.
- combine_fts_leaf_plans unions indexed hits with flat hits from unindexed
  fragments and takes the top-k with a score-only SortExec, which does not break
  ties deterministically. Sort on row_id as well, matching the MultiMatch arm.
The collector settled a score tie on the row address alone, but element
documents of one row all carry that row's address. Their order is decided by
doc_index, which the walk never sees, so evicting on the address key dropped an
arbitrary element and no later sort could bring it back. List-element queries
always take this path, and a prewarmed index attaches the address projection, so
any two equally scored elements of one row could come back in either order.

Give the collector one rule for every mode: compare candidates on
`(score, order_key)` and retain, rather than rank, whatever compares equal.
Distinct row addresses make that unreachable, so a row-granularity walk behaves
exactly as before; a deferred walk has a constant key, which degenerates to the
score-only band it already kept; and elements of one row are now retained for
the resolved re-selection. The band is capped only when the walk defers
addresses, since the address-ordered band is bounded by one row's tied element
count rather than by the corpus.

The replacement path also stopped reusing frequency slots: it took a new slot
for the newcomer before releasing the displaced one, so a deferred walk did one
allocation and one free per improving candidate, which is what FrequencySlots
exists to prevent. Decide the displaced entry's fate from the entry the pop
exposes, then reuse its slot in place unless it actually joins the band.
The FTS plan top-k sorted on `(_score DESC, _rowid ASC)`. For list-element
granularity `_rowid` repeats across the elements of one row, so the key is not
total and `SortExec::with_fetch` is a non-stable top-k fed by a union of
independent plans. A row with two tied elements returned either of them at
limit=1, and top-1 was not a prefix of top-2.

Append `_doc_index ASC` when the plan carries element coordinates, and share the
key between the indexed-plus-flat union and the MultiMatch arm, which built it
verbatim.
Three problems with how the merge handled a broad tied query, where quantized
doc lengths give thousands of documents bit-identical BM25 and every partition
overflows its tie band at once.

- The overflow retries ran serially behind a pipeline that loads and scores
  partitions concurrently, so a 64-partition index did 64 sequential address
  reads, posting reloads and WAND re-walks. Run them with the same concurrency
  as the main pass.
- The merge tie band was the one buffer that grew with the number of
  partitions rather than with `limit`. Past `limit + SCORE_FLOOR_BUFFER` the
  merge now resolves what it holds, keeps the exact top-k and starts a fresh
  band, which stays exact because every later candidate is then compared
  against the true k-th of everything seen so far. It costs address reads for
  partitions that may not survive, so it only runs past the bound.
- A retry doubles a partition's work and reported nothing. Add
  `fts_score_floor_overflows` and `fts_peak_buffered_candidates`, the plain-path
  counterparts of the compound collector's metrics.

Regression coverage for these and for the two ordering fixes before them: an
element-granularity index whose winning element is visited after the heap fills,
the merge band staying under its bound across six tied partitions, every
partition being retried at once, and the legacy merge settling ties by row_id.

The legacy merge also stops admitting NaN scores. `OrderedFloat` compares with
`total_cmp`, which ranks NaN above every real score, so a NaN candidate that
reached an under-filled heap used to come back first. Rejecting it matches what
the WAND collector has always done once full.
A list-element column where every element holds the same term, with some
fragments indexed and one appended after indexing, so the scan takes the top-k
over the union of indexed and flat hits. One row_id covers several results and
only doc_index separates them, which is the case the plan sort key missed.
Resolving an element document's key read its coordinate columns straight from
the docs file, one `read_ranges` per call. The cross-partition merge compacts
its tie band after every partition once past the bound, re-resolving the
candidates of every partition merged so far, so partition i issued up to i
coordinate reads. A list-element query over 64 partitions with a broad tied term
came to roughly 2000 reads.

Read the columns once per partition and cache them, the way row addresses
already are, which also stops the resident path paying for the merge resolving
its survivors and then resolving them again for the final ranking. Row
granularity was never affected: it short-circuits before this.
The retained band was capped only when the walk deferred row addresses. An
address-ordered walk left it uncapped, justified as bounded by one row's tied
element count, but nothing bounds how many elements a list row holds. A row with
200k elements all carrying the query term at the same length made the walk
retain 200k candidates, each with its own frequency allocation, per partition
per query. It was the last uncapped buffer on this path and had no overflow
signal, so nothing reported it either.

Cap both modes at `limit + SCORE_FLOOR_BUFFER`. Past the cap the heap keeps
ranking on the posting DocId, so the documents the result order would pick still
reach the final selection while the rest of the unrankable group is dropped. To
make that fall-through possible the ordering key splits in two: a rank key
(score, row address) that says whether the walk can tell two candidates apart,
and the DocId that makes the heap order total. The DocId is consulted only past
the cap, because it matches `doc_index` for indexes this writer produces but is
not a format guarantee.

A full band while already ordering by address cannot be recovered by rereading,
so it is reported through `fts_element_band_truncations` rather than driving the
retry, which stays reserved for the deferred case it can actually fix.
- `tie_order_key`'s default returns the document key, not a constant; only the
  modern adapter returns a constant when it defers addresses. Note what that
  means for wand unit tests over a DocSet without row_ids: they rank where
  production would retain, so they do not exercise the band.
- The NaN guard does not align the two layers. `TopKCollector::insert` still
  admits a NaN score into an under-filled heap, which the merge then drops, so a
  partition can contribute one candidate fewer than `limit`. Pre-existing, and
  left alone.
- The merge-bound test asserted the right thing but said the wrong one: the peak
  is the compacted band plus one partition's contribution.

Also note at the MultiMatch arm that its aggregate groups on `_rowid` alone, so
the sort key has no coordinate to pick up and element ordering within a
MultiMatch row is a separate, pre-existing gap.
Upstream lance-format#7975 moved the indexed-plus-flat sort into combine_fts_leaf_plans and
routed a flat-only plan that carries a limit through the same site. That top-k
now uses the full FTS key too, so cover it: with no index every hit is scored on
the flat path, and only the sort key decides which of the tied rows survive the
fetch. The test fails on a score-only sort.

Retarget the comment at that site as well, which still described the union alone.
@sbrunk
sbrunk force-pushed the fts-deterministic-topk-tiebreak branch from 03cbcb6 to eb66f95 Compare August 11, 2026 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-index Vector index, linalg, tokenizer bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant