feat(fts): add BM25F cross-field search#7905
Conversation
github-actions
Bot
added
A-python
📝 WalkthroughWalkthroughAdds BM25F-style ChangesCombined fields full-text search
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant Scanner
participant CombinedFieldsQueryExec
participant InvertedIndex
participant MaxscoreSearch
Client->>Scanner: submit combined_fields query
Scanner->>CombinedFieldsQueryExec: create execution plan
CombinedFieldsQueryExec->>InvertedIndex: open target columns and validate tokenizers
InvertedIndex-->>CombinedFieldsQueryExec: return segments and statistics
CombinedFieldsQueryExec->>MaxscoreSearch: search postings with scorer and prefilter
MaxscoreSearch-->>CombinedFieldsQueryExec: return top-k row ids and scores
CombinedFieldsQueryExec-->>Client: emit FTS result batch
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
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/builder.rs (1)
349-365: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
sort_docs_by_row_id()here blocks the async runtime; wrap inspawn_cpulikemerge_all_tail_partitions.This is the same reorder operation that
merge_all_tail_partitionsexplicitly offloads tospawn_cpu(with a comment justifying it), but here it runs inline in the async task. For a partition merged from several existing segments (up to the worker memory limit), this can be an O(n log n) sort plus a full doc-set/posting-list rebuild — substantial CPU work that starves the runtime thread for the duration.🔧 Proposed fix
async fn write_new_partition( &mut self, dest_store: &dyn IndexStore, mut builder: InnerBuilder, ) -> Result<Vec<IndexFile>> { let partition_id = self.next_partition_id() | self.fragment_mask.unwrap_or(0); builder.set_id(partition_id); // A partition merged from several existing segments is a concatenation // of their doc runs; restore a global row_id order so read pruning keeps - // working after updates (a no-op when it is already ascending). - builder.sort_docs_by_row_id(); + // working after updates (a no-op when it is already ascending). Offload + // to spawn_cpu, like merge_all_tail_partitions, since this can rebuild + // the whole doc set and every posting list. + builder = spawn_cpu(move || { + builder.sort_docs_by_row_id(); + builder + }) + .await?; let files = builder .write_to(dest_store, self.partition_write_target()) .await?;🤖 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/builder.rs` around lines 349 - 365, Update write_new_partition to offload builder.sort_docs_by_row_id() through spawn_cpu, matching the existing merge_all_tail_partitions pattern, and await the returned result before calling write_to. Preserve the partition ID assignment and subsequent file-writing flow.
🟡 Other comments (4)
rust/lance/src/dataset/tests/dataset_index.rs-1090-1090 (1)
1090-1090: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDoc comment overstates ordering guarantee.
This says results come back "in score-descending order", but
test_fts_combined_fields_boost_ranking(Lines 1006-1007) explicitly notes FTS batch order is not a guaranteed ranking. All callers here wrap the result in aHashSet, so there's no functional impact, but the comment is misleading — align it withfts_result_id_scores("in result order").As per coding guidelines: "Ensure doc comments match actual semantics".
📝 Proposed wording fix
-/// Run a full-text query and return the matched `id`s in score-descending order. +/// Run a full-text query and return the matched `id`s in result order.🤖 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/src/dataset/tests/dataset_index.rs` at line 1090, Update the doc comment for the full-text query helper near `fts_result_id_scores` to describe returned IDs as being in result order rather than score-descending order. Keep the implementation unchanged and align the wording with the actual FTS ordering semantics.Source: Coding guidelines
rust/lance/benches/fts/run_combined_fields_compare.sh-24-25 (1)
24-25: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFail fast if the repo root can't be resolved.
With only
set -uo pipefail(no-e), a failinggit rev-parseleavesREPO_ROOTempty;cd "$REPO_ROOT"then fails silently and the script proceeds, after which Line 66 runsrm -f "$REPO_ROOT"/target/release/deps/...against an absolute/target/...path. Guard thecd.🛡️ Proposed guard
-REPO_ROOT="$(git -C "$SCRIPT_DIR" rev-parse --show-toplevel)" -cd "$REPO_ROOT" +REPO_ROOT="$(git -C "$SCRIPT_DIR" rev-parse --show-toplevel)" || { echo "ERROR: not a git repo" >&2; exit 1; } +cd "$REPO_ROOT" || { echo "ERROR: cannot cd to $REPO_ROOT" >&2; exit 1; }🤖 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/benches/fts/run_combined_fields_compare.sh` around lines 24 - 25, Update the repository-root setup using REPO_ROOT and the following cd command so failure to resolve or enter the repository root immediately terminates the script; preserve the existing resolved-root behavior for successful execution and prevent later commands from running with an empty root.Source: Linters/SAST tools
rust/lance-index/src/scalar/inverted/query.rs-603-616 (1)
603-616: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConsider rejecting duplicate columns in
try_new.
columnsisn't checked for duplicates. A caller passing e.g.["title", "title"]will silently double the effective weight/length contribution of that column in the BM25F blend (each occurrence gets its own default boost of1.0, and downstream blending presumably sums per-column contributions), producing skewed scores without any error.🛡️ Proposed validation
pub fn try_new(terms: String, columns: Vec<String>) -> Result<Self> { if columns.is_empty() { return Err(Error::invalid_input( "Cannot create CombinedFieldsQuery with no columns".to_string(), )); } + let mut seen = std::collections::HashSet::with_capacity(columns.len()); + if let Some(dup) = columns.iter().find(|c| !seen.insert(c.as_str())) { + return Err(Error::invalid_input(format!( + "Duplicate column '{}' in combined_fields query columns", + dup + ))); + } let boosts = vec![Self::MIN_BOOST; columns.len()];🤖 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/query.rs` around lines 603 - 616, Update CombinedFieldsQuery::try_new to validate that columns contains no duplicate names before constructing boosts and returning the query. Return an invalid-input error identifying the duplicate column, while preserving the existing empty-columns validation and normal behavior for unique columns.rust/lance-index/src/scalar/inverted/builder.rs-4600-4694 (1)
4600-4694: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winSort worker flushes before writing
flush()writesself.builderas-is, whileprocess_document()appendsrow_ids in arrival order. A worker that hits the memory limit on shuffled input can emit an unsorted partition and miss therow_idpruning fast path; callsort_docs_by_row_id()here or make the monotonic-input guarantee explicit.🤖 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/builder.rs` around lines 4600 - 4694, The worker flush path writes documents in arrival order, so shuffled input can produce unsorted partitions. Update the flush implementation that writes self.builder to invoke sort_docs_by_row_id() immediately before writing, preserving the existing behavior for all other flush processing.
🧹 Nitpick comments (2)
rust/lance/src/io/exec/fts.rs (2)
824-831: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: record scorer-build timing for parity with
MatchQueryExec.
FtsIndexMetrics::record_scorer_buildexists but isn't invoked on this path, so thescorer_build_msgauge stays unset forcombined_fields. Wrapping thebuild_combined_bm25_scorercall in a timer keeps observability consistent across FTS execs.🤖 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/src/io/exec/fts.rs` around lines 824 - 831, In the fallback branch of the scorer selection around build_combined_bm25_scorer, measure the duration of scorer construction and record it through FtsIndexMetrics::record_scorer_build. Leave the preset_base_scorer path unchanged and ensure the existing async error propagation remains intact.
767-773: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrefer
.ok_or_else(...)so the error value isn't built on the success path. Both sites pass an eagerly-constructedDataFusionError(withformat!/to_string) to.ok_or, allocating even when theOptionisSome.
rust/lance/src/io/exec/fts.rs#L767-L773: replace.ok_or(DataFusionError::Execution(format!("No Inverted index found for column {}", column)))with.ok_or_else(|| DataFusionError::Execution(format!(...))).rust/lance/src/io/exec/fts.rs#L815-L820: replace.ok_or(DataFusionError::Execution("combined_fields query has no target columns".to_string()))with.ok_or_else(|| DataFusionError::Execution(...)).🤖 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/src/io/exec/fts.rs` around lines 767 - 773, Replace the eager error construction with lazy closures at both sites in rust/lance/src/io/exec/fts.rs:767-773 and rust/lance/src/io/exec/fts.rs:815-820. Update the load_segments inverted-index lookup and the combined_fields target-columns lookup to use ok_or_else while preserving their existing DataFusionError messages.
🤖 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/parser.rs`:
- Around line 114-161: Update CombinedFieldsQuery::from_json to distinguish
missing optional fields from present values with invalid types: reject any
present boost that is not an array of numbers, and reject any present operator
that is not a string, using descriptive invalid-input errors. Preserve the
existing defaults only when boost or operator is absent, while retaining current
parsing and validation for correctly typed values.
---
Outside diff comments:
In `@rust/lance-index/src/scalar/inverted/builder.rs`:
- Around line 349-365: Update write_new_partition to offload
builder.sort_docs_by_row_id() through spawn_cpu, matching the existing
merge_all_tail_partitions pattern, and await the returned result before calling
write_to. Preserve the partition ID assignment and subsequent file-writing flow.
---
Other comments:
In `@rust/lance-index/src/scalar/inverted/builder.rs`:
- Around line 4600-4694: The worker flush path writes documents in arrival
order, so shuffled input can produce unsorted partitions. Update the flush
implementation that writes self.builder to invoke sort_docs_by_row_id()
immediately before writing, preserving the existing behavior for all other flush
processing.
In `@rust/lance-index/src/scalar/inverted/query.rs`:
- Around line 603-616: Update CombinedFieldsQuery::try_new to validate that
columns contains no duplicate names before constructing boosts and returning the
query. Return an invalid-input error identifying the duplicate column, while
preserving the existing empty-columns validation and normal behavior for unique
columns.
In `@rust/lance/benches/fts/run_combined_fields_compare.sh`:
- Around line 24-25: Update the repository-root setup using REPO_ROOT and the
following cd command so failure to resolve or enter the repository root
immediately terminates the script; preserve the existing resolved-root behavior
for successful execution and prevent later commands from running with an empty
root.
In `@rust/lance/src/dataset/tests/dataset_index.rs`:
- Line 1090: Update the doc comment for the full-text query helper near
`fts_result_id_scores` to describe returned IDs as being in result order rather
than score-descending order. Keep the implementation unchanged and align the
wording with the actual FTS ordering semantics.
---
Nitpick comments:
In `@rust/lance/src/io/exec/fts.rs`:
- Around line 824-831: In the fallback branch of the scorer selection around
build_combined_bm25_scorer, measure the duration of scorer construction and
record it through FtsIndexMetrics::record_scorer_build. Leave the
preset_base_scorer path unchanged and ensure the existing async error
propagation remains intact.
- Around line 767-773: Replace the eager error construction with lazy closures
at both sites in rust/lance/src/io/exec/fts.rs:767-773 and
rust/lance/src/io/exec/fts.rs:815-820. Update the load_segments inverted-index
lookup and the combined_fields target-columns lookup to use ok_or_else while
preserving their existing DataFusionError messages.
🪄 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: 22043023-3436-4aa3-9f86-7dad52ddd18b
📒 Files selected for processing (20)
docs/src/quickstart/full-text-search.mdpython/python/lance/lance/__init__.pyipython/python/lance/query.pypython/python/tests/test_scalar_index.pypython/src/dataset.rsrust/lance-index/src/scalar/inverted.rsrust/lance-index/src/scalar/inverted/builder.rsrust/lance-index/src/scalar/inverted/combined.rsrust/lance-index/src/scalar/inverted/index.rsrust/lance-index/src/scalar/inverted/parser.rsrust/lance-index/src/scalar/inverted/query.rsrust/lance-index/src/scalar/inverted/scorer.rsrust/lance-index/src/scalar/inverted/tokenizer.rsrust/lance/Cargo.tomlrust/lance/benches/fts/LuceneCombinedFieldsBench.javarust/lance/benches/fts/combined_fields_compare.rsrust/lance/benches/fts/run_combined_fields_compare.shrust/lance/src/dataset/scanner.rsrust/lance/src/dataset/tests/dataset_index.rsrust/lance/src/io/exec/fts.rs
Score multiple text columns as one virtual field (Lucene CombinedFieldQuery / BM25F blend) instead of the per-field max fusion of MultiMatch. Adds the query type + serde + JSON parser, CombinedFieldsBM25Scorer, CombinedFieldsQueryExec, Python bindings, docs, and a Lance-vs-Lucene + brute-force validation harness. Cross-field document lengths are read per candidate via DocSet::doc_length_by_row_id (no full-docs scan).
Merge worker tail partitions in row_id order (stable sort + consistent doc-id remap of docs and posting lists) so each partition's row_ids come out strictly ascending. Internal doc-id relabel only: BM25 scores/results unchanged, no format or metadata change, old unordered indexes stay valid. Enables row-id block-skipping for combined_fields read pruning.
Term-at-a-time MAXSCORE with a constant per-term ceiling (idf*(k1+1)) prunes scoring for non-competitive candidates; a lazy cross-column cursor (FastPostingSource/LazyTerm) skips posting blocks by row_id when a partition's row_ids are ascending, falling back to a bit-identical full scan otherwise. Top-k scores are bit-exact vs the exact scan. Adds a --skew/--perf bench arm.
sort_docs_by_row_id in write_new_partition ran inline on the tokio runtime thread. On the merge_existing_segments (optimize/update) path a partition can reach the worker memory limit, so its O(n log n) sort plus doc-set and posting rebuild can starve the runtime thread. Offload it to spawn_cpu, matching merge_all_tail_partitions. The sort is unchanged and deterministic, so results and scores are identical.
1daeae1 to
6debf48
Compare
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
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/index.rs (1)
6796-6804: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
row_ids_ascendingcache is not invalidated on mutation, unlikenorms.
append(andremapat Lines 6690-6716) mutaterow_idsbut never reset the memoizedrow_ids_ascendingcell, whereas both correctly callinvalidate_norms(). Todayrow_ids_strictly_ascending()is only invoked on loaded, immutableArc<DocSet>s during search, so this is not yet reachable — but the asymmetry is a latent correctness trap: any future caller that queries the ascending property and then appends/remaps would read a stale answer, and combined-fields fast-path eligibility hinges on this exact flag. Mirroring thenormsguard keeps the invariant robust.🛡️ Suggested guard (mirror invalidate_norms)
fn invalidate_row_ids_ascending(&mut self) { if self.row_ids_ascending.get().is_some() { self.row_ids_ascending = Arc::new(std::sync::OnceLock::new()); } }Call it from
appendandremapalongsideinvalidate_norms().🤖 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 6796 - 6804, Invalidate the memoized row_ids_ascending cache whenever DocSet mutations change row_ids. Add an invalidate_row_ids_ascending helper mirroring invalidate_norms, and call it from both append and remap alongside invalidate_norms so future ascending-order queries recompute their result.
🟡 Other comments (3)
rust/lance/benches/fts/run_combined_fields_compare.sh-24-25 (1)
24-25: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the
cdagainst an emptyREPO_ROOT.If
git rev-parsefails,REPO_ROOTis empty and, with-enot set,cd ""is a no-op that leaves the script running from the caller's directory, sorm -rf "$WORK"and the build run in an unexpected place.🛠️ Proposed fix
-REPO_ROOT="$(git -C "$SCRIPT_DIR" rev-parse --show-toplevel)" -cd "$REPO_ROOT" +REPO_ROOT="$(git -C "$SCRIPT_DIR" rev-parse --show-toplevel)" || { echo "ERROR: not a git checkout" >&2; exit 1; } +cd "$REPO_ROOT" || { echo "ERROR: cd $REPO_ROOT failed" >&2; exit 1; }🤖 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/benches/fts/run_combined_fields_compare.sh` around lines 24 - 25, Update the repository-root setup in run_combined_fields_compare.sh so failure to resolve REPO_ROOT stops execution before the cd and subsequent workspace or build operations. Validate that REPO_ROOT is non-empty and make the cd fail explicitly when the value is invalid.Source: Linters/SAST tools
rust/lance/benches/fts/run_combined_fields_compare.sh-67-72 (1)
67-72: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFail fast when the Lance bench build fails.
set -eis not enabled andcargo bench ... --no-runhas no failure check, so a build error falls through to thefindon Line 68, leavesLANCE_BINempty, and Line 72 then tries to execute an empty command — masking the real failure. Check the build result and thatLANCE_BINresolves to an executable.🛠️ Proposed fix
-cargo bench -p lance --bench combined_fields_compare --no-run +cargo bench -p lance --bench combined_fields_compare --no-run \ + || { echo "ERROR: cargo bench build failed" >&2; exit 1; } LANCE_BIN="$(find "$REPO_ROOT/target/release/deps" -maxdepth 1 -type f -perm -111 \ -name 'combined_fields_compare-*' ! -name '*.d' -exec ls -t {} + | head -1)" +[ -x "$LANCE_BIN" ] || { echo "ERROR: combined_fields_compare binary not found" >&2; exit 1; }🤖 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/benches/fts/run_combined_fields_compare.sh` around lines 67 - 72, Update the benchmark setup around the cargo bench build and LANCE_BIN resolution to fail immediately when compilation fails or no executable is found. Check the result of `cargo bench -p lance --bench combined_fields_compare --no-run`, then validate that `LANCE_BIN` is non-empty and executable before invoking it; report a clear error and exit nonzero when either check fails.rust/lance/src/dataset/tests/dataset_index.rs-1024-1088 (1)
1024-1088: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
HashSet-based ID comparisons across three combined-fields tests can mask duplicate-row emission. Each site matches a document via two different column postings for the query, but the assertion only compares an idHashSet(or nothing) against expected ids, never the result count, so a bug that emits the same row twice would pass silently.
rust/lance/src/dataset/tests/dataset_index.rs#L1024-L1088: at Lines 1070-1073, assertfts_result_ids(...).len() == 3before converting to the id set — this is the case whose own comment ("matches once") documents the exact behavior left unverified.rust/lance/src/dataset/tests/dataset_index.rs#L866-L955: at Lines 936-954, add a length check on the rawVec<i32>before/alongside eachas_set(...)comparison for both the AND and OR assertions.rust/lance/src/dataset/tests/dataset_index.rs#L1222-L1302: at Lines 1287-1294, assertactual.len() == expected_ids.len()before derivingactual_idsas aHashSet.🤖 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/src/dataset/tests/dataset_index.rs` around lines 1024 - 1088, Prevent HashSet assertions from masking duplicate result rows in the three combined-fields tests. In rust/lance/src/dataset/tests/dataset_index.rs:1024-1088, capture the raw fts_result_ids result and assert its length is 3 before converting to a set; in rust/lance/src/dataset/tests/dataset_index.rs:866-955, assert raw result lengths for both AND and OR cases before each as_set comparison; in rust/lance/src/dataset/tests/dataset_index.rs:1222-1302, assert actual.len() equals expected_ids.len() before deriving actual_ids.
🧹 Nitpick comments (2)
rust/lance/src/dataset/tests/dataset_index.rs (1)
1598-1692: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAssert the
Error::invalid_inputkind too
The test should check the error kind as well as the message;validate_combined_tokenizersalready emitsError::invalid_input, so this will catch any future wrapping that still preserves the text but loses the typed contract.🤖 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/src/dataset/tests/dataset_index.rs` around lines 1598 - 1692, Update test_fts_combined_fields_tokenizer_validation to assert that the rejected full-text search returns Error::invalid_input, not only a matching message. Preserve the existing tokenizer and combined_fields message checks while validating the typed error kind from the result returned by the scan execution.Source: Coding guidelines
rust/lance-index/src/scalar/inverted/query.rs (1)
570-593: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a rustdoc example for the new public API.
CombinedFieldsQueryis a new public struct but its doc comment has no runnable example, only prose and links. As per coding guidelines, "Document all public APIs with examples and links to relevant structs and methods; keep examples synchronized with actual signatures."📝 Suggested addition
/// Per-column `boosts` follow Lucene's `CombinedFieldQuery`: every weight must be /// `>= 1` (fractional weights allowed) so the combined length norm stays /// additive. +/// +/// # Example +/// +/// ``` +/// use lance_index::scalar::inverted::query::CombinedFieldsQuery; +/// +/// let query = CombinedFieldsQuery::try_new( +/// "hello world".to_string(), +/// vec!["title".to_string(), "body".to_string()], +/// )? +/// .try_with_boosts(vec![2.0, 1.0])?; +/// # Ok::<(), lance_core::Error>(()) +/// ``` #[derive(Debug, Clone, PartialEq)] pub struct CombinedFieldsQuery {🤖 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/query.rs` around lines 570 - 593, Add a runnable Rustdoc code example to the public CombinedFieldsQuery documentation, using its actual try_new and try_with_boosts signatures, importing the required symbols, and returning the appropriate result type so the example compiles and demonstrates configuring columns and boosts.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/index.rs`:
- Around line 6796-6804: Invalidate the memoized row_ids_ascending cache
whenever DocSet mutations change row_ids. Add an invalidate_row_ids_ascending
helper mirroring invalidate_norms, and call it from both append and remap
alongside invalidate_norms so future ascending-order queries recompute their
result.
---
Other comments:
In `@rust/lance/benches/fts/run_combined_fields_compare.sh`:
- Around line 24-25: Update the repository-root setup in
run_combined_fields_compare.sh so failure to resolve REPO_ROOT stops execution
before the cd and subsequent workspace or build operations. Validate that
REPO_ROOT is non-empty and make the cd fail explicitly when the value is
invalid.
- Around line 67-72: Update the benchmark setup around the cargo bench build and
LANCE_BIN resolution to fail immediately when compilation fails or no executable
is found. Check the result of `cargo bench -p lance --bench
combined_fields_compare --no-run`, then validate that `LANCE_BIN` is non-empty
and executable before invoking it; report a clear error and exit nonzero when
either check fails.
In `@rust/lance/src/dataset/tests/dataset_index.rs`:
- Around line 1024-1088: Prevent HashSet assertions from masking duplicate
result rows in the three combined-fields tests. In
rust/lance/src/dataset/tests/dataset_index.rs:1024-1088, capture the raw
fts_result_ids result and assert its length is 3 before converting to a set; in
rust/lance/src/dataset/tests/dataset_index.rs:866-955, assert raw result lengths
for both AND and OR cases before each as_set comparison; in
rust/lance/src/dataset/tests/dataset_index.rs:1222-1302, assert actual.len()
equals expected_ids.len() before deriving actual_ids.
---
Nitpick comments:
In `@rust/lance-index/src/scalar/inverted/query.rs`:
- Around line 570-593: Add a runnable Rustdoc code example to the public
CombinedFieldsQuery documentation, using its actual try_new and try_with_boosts
signatures, importing the required symbols, and returning the appropriate result
type so the example compiles and demonstrates configuring columns and boosts.
In `@rust/lance/src/dataset/tests/dataset_index.rs`:
- Around line 1598-1692: Update test_fts_combined_fields_tokenizer_validation to
assert that the rejected full-text search returns Error::invalid_input, not only
a matching message. Preserve the existing tokenizer and combined_fields message
checks while validating the typed error kind from the result returned by the
scan execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 21aba1e3-5b8c-4f48-bde1-058b7461b911
📒 Files selected for processing (20)
docs/src/quickstart/full-text-search.mdpython/python/lance/lance/__init__.pyipython/python/lance/query.pypython/python/tests/test_scalar_index.pypython/src/dataset.rsrust/lance-index/src/scalar/inverted.rsrust/lance-index/src/scalar/inverted/builder.rsrust/lance-index/src/scalar/inverted/combined.rsrust/lance-index/src/scalar/inverted/index.rsrust/lance-index/src/scalar/inverted/parser.rsrust/lance-index/src/scalar/inverted/query.rsrust/lance-index/src/scalar/inverted/scorer.rsrust/lance-index/src/scalar/inverted/tokenizer.rsrust/lance/Cargo.tomlrust/lance/benches/fts/LuceneCombinedFieldsBench.javarust/lance/benches/fts/combined_fields_compare.rsrust/lance/benches/fts/run_combined_fields_compare.shrust/lance/src/dataset/scanner.rsrust/lance/src/dataset/tests/dataset_index.rsrust/lance/src/io/exec/fts.rs
- query.rs: CombinedFieldsQuery::try_new rejects duplicate columns (a duplicate double-counts that field's postings and sum_total_term_freq, skewing the BM25F blend); covered by the validation test. - fts.rs: record scorer_build timing on the combined path, matching the other FTS exec paths. - builder.rs: document the flush row_id ordering invariant (flushed partitions inherit scan order; a non-monotonic input only costs the read-pruning fast path, never correctness; no inline sort here). - run_combined_fields_compare.sh: fail hard if the repo root cannot be resolved so the later rm never targets /target. - dataset_index.rs: fix a test helper doc comment (result order, not score-descending).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
rust/lance/src/io/exec/fts.rs (1)
727-727: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn execution errors instead of panicking on internal assumptions.
Line 727 and Lines 802-804 use
unwrap/expectin library execution code. Preserve the invariant checks, but convert failures toDataFusionError::Internalwith context rather than panicking.Proposed fix
- let src = children.pop().unwrap(); + let Some(src) = children.pop() else { + return Err(DataFusionError::Internal( + "Expected exactly one prefilter child".to_string(), + )); + }; ... - Arc::get_mut(&mut pre_filter) - .expect("prefilter just created") - .set_deleted_fragments(deleted_fragments); + let strong_count = Arc::strong_count(&pre_filter); + Arc::get_mut(&mut pre_filter) + .ok_or_else(|| DataFusionError::Internal(format!( + "Could not set deleted fragments: prefilter strong_count={strong_count}" + )))? + .set_deleted_fragments(deleted_fragments);As per coding guidelines, “Never use
.unwrap(),.expect(),panic!(), orassert!()in library code for fallible operations.”Also applies to: 802-804
🤖 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/src/io/exec/fts.rs` at line 727, Update the execution logic around the children collection and the related lines 802-804 to replace unwrap/expect calls with fallible handling that returns DataFusionError::Internal containing clear invariant context. Preserve the existing invariant checks and successful execution behavior, but propagate these errors instead of allowing panics.Source: Coding guidelines
rust/lance-index/src/scalar/inverted/query.rs (1)
1261-1293: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the invalid-input variant and message in validation tests.
These cases rely on
.is_err()/.is_ok(), so tests can pass with the wrong error type or message. Assert the invalid-input variant and stable message content for empty columns, duplicates, boost-count mismatches, and invalid boosts.🤖 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/query.rs` around lines 1261 - 1293, Strengthen test_combined_fields_query_validation by matching the returned validation errors instead of only checking is_err/is_ok. Assert the invalid-input variant and stable message content for empty columns, duplicate columns, boost-count mismatches, and boosts below 1 or NaN, while retaining the successful fractional-boost assertion.Source: Coding guidelines
rust/lance-index/src/scalar/inverted/builder.rs (1)
1142-1149: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPropagate posting-list rebuild errors instead of panicking.
sort_docs_by_row_iduses.expect(...)in library code, andold_to_new[old_doc_id]can also panic on inconsistent posting data. ReturnResult<()>, validate the document ID, and propagate errors through the merge/write callers with posting-list and partition context.🤖 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/builder.rs` around lines 1142 - 1149, Update sort_docs_by_row_id to return Result<()> instead of panicking, validate each old_doc_id before indexing old_to_new, and propagate posting-list iteration or validation errors. Thread the Result through its merge/write callers, adding posting-list and partition context to propagated errors while preserving successful rebuild behavior.Source: Coding guidelines
rust/lance/benches/fts/run_combined_fields_compare.sh (6)
35-35: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not recursively delete an arbitrary
WORKpath.
WORKis environment-controlled, so a typo or unsafe override can erase an existing directory before the benchmark runs. Use a newly created temporary directory, or refuse paths outside an explicitly dedicated workspace.🤖 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/benches/fts/run_combined_fields_compare.sh` at line 35, Update the WORK setup in the benchmark script to avoid recursively deleting an environment-controlled path. Create and use a newly generated temporary directory, or validate WORK against an explicitly dedicated workspace before allowing cleanup; preserve the subsequent mkdir and benchmark flow.
90-92: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject mismatched result-file lengths instead of truncating them.
n = min(...)silently ignores missing trailing queries from any runner. A partial Lance or Lucene output can therefore be scored against only the common prefix and potentially pass the gate. Require all three files to contain the same number of rows before computing metrics.Suggested fix
lance, lucene, truth = rows("lance_topk.txt"), rows("lucene_topk.txt"), rows("truth.txt") -n = min(len(lance), len(lucene), len(truth)) +lengths = (len(lance), len(lucene), len(truth)) +if len(set(lengths)) != 1: + raise SystemExit(f"row-count mismatch: lance={lengths[0]}, lucene={lengths[1]}, truth={lengths[2]}") +n = len(truth)🤖 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/benches/fts/run_combined_fields_compare.sh` around lines 90 - 92, Update the result-length setup in the rows-loading comparison flow to require lance, lucene, and truth to have identical row counts; reject or fail clearly on any mismatch before computing metrics, and remove the min-based truncation so scoring always uses complete outputs.
28-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate all environment-provided benchmark parameters.
Values such as
MIN_OK=-1can make the gate pass regardless of quality, while invalid or non-positive corpus values are only rejected later with less context. Validate integer knobs and require0 <= MIN_OK <= 1before creating the work directory.As per coding guidelines, validate inputs at API boundaries and reject invalid values with descriptive errors.
🤖 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/benches/fts/run_combined_fields_compare.sh` around lines 28 - 34, Validate the environment-derived parameters DOCS, VOCAB, QUERIES, and K as positive integers, and validate MIN_OK as a numeric value within 0 through 1, before creating WORK in the benchmark script. Emit descriptive errors and exit immediately for invalid values; leave valid parameter handling unchanged.Source: Coding guidelines
67-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResolve the benchmark binary from Cargo’s resolved target directory.
cargo bench --no-runcan place the artifact outside"$REPO_ROOT"/targetwhenCARGO_TARGET_DIRortarget-diris set, soLANCE_BINcan end up empty after a successful build.🤖 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/benches/fts/run_combined_fields_compare.sh` around lines 67 - 70, Update the benchmark binary lookup in run_combined_fields_compare.sh to use Cargo’s resolved target directory rather than hardcoding $REPO_ROOT/target. Ensure both stale-artifact removal and the find operation use the same resolved directory, preserving selection of the newest executable combined_fields_compare artifact.
52-61: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCheck the analysis jar before setting
LUCENE_CP.CORE_JARis re-found after the build, butANALYSIS_JARisn’t. If the analysis jar is missing, the script keeps going with a malformed classpath; re-check both jars after the Gradle step and fail explicitly if either is still absent.🤖 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/benches/fts/run_combined_fields_compare.sh` around lines 52 - 61, Update the Lucene jar discovery flow in the script around CORE_JAR, ANALYSIS_JAR, and the Gradle build so both jars are re-found after building and validated before assigning LUCENE_CP. If either jar remains missing, print an explicit error and exit instead of continuing with an incomplete classpath.
45-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle invalid
JAVA_HOMEand preflight both tools. IfJAVA_HOMEpoints to a missing JDK, this keeps using that broken path instead of falling back toPATH. It also only checksjava, even thoughjavacis required later, and it never enforces the documented JDK 21+ minimum.🤖 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/benches/fts/run_combined_fields_compare.sh` around lines 45 - 48, Update the Java tool initialization and preflight in run_combined_fields_compare.sh to use JAVA_HOME only when its java and javac executables exist, otherwise fall back to PATH. Validate both "$JAVA" and "$JAVAC" before continuing, and enforce the documented JDK 21-or-newer requirement using the existing version output flow.
🧹 Nitpick comments (1)
rust/lance-index/src/scalar/inverted/query.rs (1)
595-601: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd examples and cross-links for the new public API.
The new public
CombinedFieldsQuerymethods need runnable Rustdoc examples and links to related types/methods, as required by the repository guidelines.Also applies to: 629-661
🤖 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/query.rs` around lines 595 - 601, Update the public CombinedFieldsQuery API documentation, including its constructor and methods in the affected range, with runnable Rustdoc examples demonstrating typical usage and appropriate cross-links to related query types and methods. Follow the repository’s existing Rustdoc conventions and ensure the examples compile as documentation 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.
Outside diff comments:
In `@rust/lance-index/src/scalar/inverted/builder.rs`:
- Around line 1142-1149: Update sort_docs_by_row_id to return Result<()> instead
of panicking, validate each old_doc_id before indexing old_to_new, and propagate
posting-list iteration or validation errors. Thread the Result through its
merge/write callers, adding posting-list and partition context to propagated
errors while preserving successful rebuild behavior.
In `@rust/lance-index/src/scalar/inverted/query.rs`:
- Around line 1261-1293: Strengthen test_combined_fields_query_validation by
matching the returned validation errors instead of only checking is_err/is_ok.
Assert the invalid-input variant and stable message content for empty columns,
duplicate columns, boost-count mismatches, and boosts below 1 or NaN, while
retaining the successful fractional-boost assertion.
In `@rust/lance/benches/fts/run_combined_fields_compare.sh`:
- Line 35: Update the WORK setup in the benchmark script to avoid recursively
deleting an environment-controlled path. Create and use a newly generated
temporary directory, or validate WORK against an explicitly dedicated workspace
before allowing cleanup; preserve the subsequent mkdir and benchmark flow.
- Around line 90-92: Update the result-length setup in the rows-loading
comparison flow to require lance, lucene, and truth to have identical row
counts; reject or fail clearly on any mismatch before computing metrics, and
remove the min-based truncation so scoring always uses complete outputs.
- Around line 28-34: Validate the environment-derived parameters DOCS, VOCAB,
QUERIES, and K as positive integers, and validate MIN_OK as a numeric value
within 0 through 1, before creating WORK in the benchmark script. Emit
descriptive errors and exit immediately for invalid values; leave valid
parameter handling unchanged.
- Around line 67-70: Update the benchmark binary lookup in
run_combined_fields_compare.sh to use Cargo’s resolved target directory rather
than hardcoding $REPO_ROOT/target. Ensure both stale-artifact removal and the
find operation use the same resolved directory, preserving selection of the
newest executable combined_fields_compare artifact.
- Around line 52-61: Update the Lucene jar discovery flow in the script around
CORE_JAR, ANALYSIS_JAR, and the Gradle build so both jars are re-found after
building and validated before assigning LUCENE_CP. If either jar remains
missing, print an explicit error and exit instead of continuing with an
incomplete classpath.
- Around line 45-48: Update the Java tool initialization and preflight in
run_combined_fields_compare.sh to use JAVA_HOME only when its java and javac
executables exist, otherwise fall back to PATH. Validate both "$JAVA" and
"$JAVAC" before continuing, and enforce the documented JDK 21-or-newer
requirement using the existing version output flow.
In `@rust/lance/src/io/exec/fts.rs`:
- Line 727: Update the execution logic around the children collection and the
related lines 802-804 to replace unwrap/expect calls with fallible handling that
returns DataFusionError::Internal containing clear invariant context. Preserve
the existing invariant checks and successful execution behavior, but propagate
these errors instead of allowing panics.
---
Nitpick comments:
In `@rust/lance-index/src/scalar/inverted/query.rs`:
- Around line 595-601: Update the public CombinedFieldsQuery API documentation,
including its constructor and methods in the affected range, with runnable
Rustdoc examples demonstrating typical usage and appropriate cross-links to
related query types and methods. Follow the repository’s existing Rustdoc
conventions and ensure the examples compile as documentation tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 4e13e1df-ce96-4e4a-8181-86fd74f9e4e2
📒 Files selected for processing (5)
rust/lance-index/src/scalar/inverted/builder.rsrust/lance-index/src/scalar/inverted/query.rsrust/lance/benches/fts/run_combined_fields_compare.shrust/lance/src/dataset/tests/dataset_index.rsrust/lance/src/io/exec/fts.rs
- index.rs: DocSet::append/remap now invalidate the memoized row_ids_ascending cell (mirroring invalidate_norms), so row_ids_strictly_ascending() cannot read a stale value after mutation. Latent today (only queried on loaded immutable DocSets) but guarded, since combined_fields read-pruning eligibility hinges on that flag. - dataset_index.rs: assert result cardinality in the combined_fields tests so a duplicate-row emission fails instead of collapsing in a HashSet compare. - query.rs: validation test asserts the InvalidInput variant and message, not just is_err().
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance/src/dataset/tests/dataset_index.rs (1)
1709-1715: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the tokenizer-mismatch error variant.
Line 1709 discards the typed error, so an unrelated error containing these words would pass. Assert
Error::InvalidInputbefore checking its message.Proposed fix
- let message = result - .expect_err("expected a tokenizer-mismatch error") - .to_string(); + let err = result.expect_err("expected a tokenizer-mismatch error"); + assert!( + matches!(&err, Error::InvalidInput { .. }), + "unexpected error variant: {err:?}" + ); + let message = err.to_string();As per coding guidelines, “Assert on both the error variant and the message content in tests.”
🤖 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/src/dataset/tests/dataset_index.rs` around lines 1709 - 1715, Update the error assertion in the tokenizer-mismatch test to preserve the typed error from the failing operation, assert that it matches the Error::InvalidInput variant, and then check the contained message for “combined_fields” and “tokenizer” instead of converting the untyped result directly to a string.Source: Coding guidelines
🟡 Other comments (1)
rust/lance-index/src/scalar/inverted/index.rs-6697-6697 (1)
6697-6697: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a remap invalidation regression test.
The new test covers
append, but not thisremapinvalidation path. Remapping can reorder row IDs; a staletruewould incorrectly enable combined-fields pruning.Add a test that memoizes ascending IDs, remaps one ID out of order, then asserts
row_ids_strictly_ascending()is false.As per coding guidelines, “Every bugfix and feature must have corresponding tests.”
🤖 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 6697, In the tests covering row-ID ordering invalidation, add a regression test for the remap path that first memoizes ascending IDs via row_ids_strictly_ascending(), remaps one ID so the order is no longer ascending, then asserts row_ids_strictly_ascending() returns false. Exercise the remap operation that triggers invalidate_row_ids_ascending(), alongside the existing append coverage.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/src/dataset/tests/dataset_index.rs`:
- Around line 1709-1715: Update the error assertion in the tokenizer-mismatch
test to preserve the typed error from the failing operation, assert that it
matches the Error::InvalidInput variant, and then check the contained message
for “combined_fields” and “tokenizer” instead of converting the untyped result
directly to a string.
---
Other comments:
In `@rust/lance-index/src/scalar/inverted/index.rs`:
- Line 6697: In the tests covering row-ID ordering invalidation, add a
regression test for the remap path that first memoizes ascending IDs via
row_ids_strictly_ascending(), remaps one ID so the order is no longer ascending,
then asserts row_ids_strictly_ascending() returns false. Exercise the remap
operation that triggers invalidate_row_ids_ascending(), alongside the existing
append coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: de3e50a4-7510-4267-8e43-3032e69d81c2
📒 Files selected for processing (3)
rust/lance-index/src/scalar/inverted/index.rsrust/lance-index/src/scalar/inverted/query.rsrust/lance/src/dataset/tests/dataset_index.rs
sbrunk
changed the title
1 participant
TL;DR
Adds a
combined_fieldsfull-text query that scores several text columns as one virtual field (BM25F / Elasticsearchcombined_fields/ LuceneCombinedFieldQuery, instead of today'sMultiMatch"best_fields" per-column-max fusion. It ships with three perf layers that take it from 4–10× slower thanbest_fieldsdown to ~parity (and faster on some workloads), verified bit-exact against an independent BM25F oracle and Apache Lucene.@Xuanwo this is related to some of the work you've been doing on FTS so it'd be great if you could have a look.
Why
Lance can already search multiple columns via
MultiMatchQuery, but it scores each column independently against its own corpus statistics and fuses by taking the max (ESbest_fields). There is no true cross-field BM25: a term rare intitlebut common inbodygets incomparable IDFs, and "term across fields" (e.g.johninfirst_name+smithinlast_name) can't be scored as if the fields were one.combined_fields(BM25F) blends the statistics so the columns behave like a single field with per-field weights.What's in the PR (3 commits, layered so each is reviewable and independently sound)
feat(fts): add combined_fields (BM25F) cross-field searchCombinedFieldsQuery+ serde/JSON,CombinedFieldsBM25Scorer,CombinedFieldsQueryExec, Python bindings, docs, Lucene + brute-force validation harness. Includes the per-candidatedl'length lookup (no full-docs scan).perf(fts): order inverted-index docs by row_id at build timerow_idsare strictly ascending. Internal doc-id relabel only.perf(fts): MAXSCORE + block-skip read pruning for combined_fieldsHow it works
BM25F blend (per query term
t, fieldsfwith weightsw_f; LuceneCombinedFieldQuery):Execution flow (commit 3):
The fast cursor only activates when a partition's
row_idsare strictly ascending, exactly what commit 2 guarantees for freshly built indexes. Old / unordered indexes transparently use the bit-identical fallback.Performance
Benchmark:
rust/lance/benches/fts/combined_fields_compare.rs(2 columns,title^2 body^1, k=10, 40 queries, 10 iters, release). Metric =combined_fieldslatency ÷best_fieldslatency (lower is better;best_fieldsis what users have today). Two query regimes: uniform (all terms similar df) and skew (Zipfian: one common + rare terms per query which is the more realistic hard case).This PR vs
best_fields¹ "before" = documented incremental measurements (v1 = feature only; dl' = commit-1 length fix only).
What each layer contributes (skew, 200k, k=10)
Two regimes, two bottlenecks: uniform is fixed by the
dl'lookup (commit 1) alone; skew needs all three layers (scoring the huge common-term candidate set, then the un-pruned posting reads, dominate in turn).Reproduce
cargo bench -p lance --bench combined_fields_compare -- --perf --docs 200000 --vocab 5000 --k 10 --perf-iters 10 --out-dir /tmp/cfcargo bench -p lance --bench combined_fields_compare -- --perf --skew --docs 200000 --vocab 2000 --k 10 --perf-iters 10 --out-dir /tmp/cfLUCENE_DIR=/path/to/lucene rust/lance/benches/fts/run_combined_fields_compare.shCorrectness
combined_fieldsintegration: 8/8 — incl.matches_brute_force_bm25f(independent exact BM25F oracle), multi-partition, cross-field AND, nulls, tokenizer validation, concatenation-identity, per-field boost.scalar::inverted: 356/356 — incl. the builder reorder across V1/V2/V3 × positions × workers{1,4} × multi-fragment (all fail-when-disabled).Compatibility
Commit 2 changes only the in-memory order of docs / doc-ids within a partition at build time. No format or
metadata.lancechange, on-disk columns are byte-structurally identical, readers never assume doc order. Old (unordered) indexes stay valid; read-pruning simply falls back on them. No public API breaks.Related / affected open PRs
NOTE(#7846)marks where it must weaken to strict<(admit equal-bound docs) once the collector sorts(score DESC, row_id ASC).docs.row_id(doc_id)access must pull from the independently-loaded address projection.query.rs,parser.rs,scanner.rs,fts.rs, tests). combined_fields would need per-granularity rules.xuanwo/stable-logical-row-address-v23(branch)Index data-model changes explored (deliberately not in this PR)
While scoping the read-pruning we evaluated three data-model levers:
row_idsglobally ascending (which would engage read-pruning without commit 2). It doesnot: the non-ascending order is a builder artifact (K parallel workers each emit an
ascending run, then tails are concatenated), independent of the id scheme. Commit 2's
build-time reorder is the fix instead and it needs no data-model change.
survivors, the constant per-term ceiling can only skip blocks a seek jumps over, not by a
block's own max score (what
best_fields' WAND does). Storing each posting block's max BM25Fcontribution would enable that, but it's an index-format change. Commits 2+3 already
reach ~parity via position-based skipping (1.4–1.8× fewer reads on skew), so this is optional
future work, not required.
single index, scored by unweighted sum. A separate data-model direction that overlaps
combined_fields' goal; noted for long-term reconciliation.
Remaining / follow-ups