feat(fts): add BM25F cross-field search - #7905
Conversation
github-actions
Bot
added
A-python
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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 combined_fields_search
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->>combined_fields_search: search postings with scorer and prefilter
combined_fields_search-->>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)
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. 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
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
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
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
|
Thank you @sbrunk for working on this, will take a look |
|
I missed the Java API. Now added in a315975 |
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.
🟡 Other comments (2)
java/src/main/java/org/lance/ipc/FullTextQuery.java-99-107 (1)
99-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDefensively copy
boosts.Unlike
columns,boostsretains and exposes the caller-owned mutable list. Mutating it after construction changes query behavior and can invalidateequals/hashCodewhile the query is in use. Store an unmodifiable copy and add a mutation regression test.As per coding guidelines, “Every bugfix and feature must have corresponding tests.”
Proposed fix
- this.boosts = boosts == null ? Optional.empty() : Optional.of(boosts); + this.boosts = + boosts == null + ? Optional.empty() + : Optional.of( + Collections.unmodifiableList(new java.util.ArrayList<>(boosts)));Also applies to: 373-401
🤖 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 `@java/src/main/java/org/lance/ipc/FullTextQuery.java` around lines 99 - 107, Update FullTextQuery.combinedFields and the CombinedFieldsQuery construction path so boosts is defensively copied and stored as an unmodifiable list, while preserving the existing null behavior. Add a regression test that mutates the caller-provided boosts list after query construction and verifies the query’s boosts and equality/hash behavior remain unchanged.Source: Coding guidelines
java/src/test/java/org/lance/ipc/LanceScannerFullTextSearchTest.java-105-114 (1)
105-114: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the expected validation error.
RuntimeExceptionaccepts unrelated scanner/JNI failures, so this test does not prove invalid-boost propagation. Capture the exception and assert a stable message fragment such ascombined_fields boost for column 'doc'or>= 1.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 `@java/src/test/java/org/lance/ipc/LanceScannerFullTextSearchTest.java` around lines 105 - 114, Update the assertThrows block in LanceScannerFullTextSearchTest to capture the thrown exception and assert that its message contains a stable invalid-boost validation fragment, such as “combined_fields boost for column 'doc'” or “>= 1”, while preserving the existing batch-draining execution path.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 `@java/src/main/java/org/lance/ipc/FullTextQuery.java`:
- Around line 353-365: Update the Javadoc for the combined-fields query near
MultiMatchQuery to document the complete Rust-side contract: state that
boosts.size() must equal columns.size(), and that a null operator defaults to
OR. Preserve the existing tokenizer, weight, and uniqueness documentation.
---
Other comments:
In `@java/src/main/java/org/lance/ipc/FullTextQuery.java`:
- Around line 99-107: Update FullTextQuery.combinedFields and the
CombinedFieldsQuery construction path so boosts is defensively copied and stored
as an unmodifiable list, while preserving the existing null behavior. Add a
regression test that mutates the caller-provided boosts list after query
construction and verifies the query’s boosts and equality/hash behavior remain
unchanged.
In `@java/src/test/java/org/lance/ipc/LanceScannerFullTextSearchTest.java`:
- Around line 105-114: Update the assertThrows block in
LanceScannerFullTextSearchTest to capture the thrown exception and assert that
its message contains a stable invalid-boost validation fragment, such as
“combined_fields boost for column 'doc'” or “>= 1”, while preserving the
existing batch-draining execution path.
🪄 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: 6f714f7b-22fa-4a46-9fdd-dd2927259cb6
📒 Files selected for processing (4)
java/lance-jni/src/blocking_scanner.rsjava/src/main/java/org/lance/ipc/FullTextQuery.javajava/src/test/java/org/lance/ipc/FullTextQueryTest.javajava/src/test/java/org/lance/ipc/LanceScannerFullTextSearchTest.java
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
The query-time BM25F direction is reasonable, but I found six independent issues that currently make this implementation unsafe to merge: reproducible result-completeness and historical-index compatibility failures, an exact top-k pruning counterexample, invalid Rust query states, a destructive benchmark path, and an unbounded CPU section on the async runtime.
| // The exec runs a single unified scan that already emits the merged | ||
| // hits sorted by score and applies the top-k limit, so (like the | ||
| // fully-indexed single-Match path) it needs no union/aggregate/sort. | ||
| FtsQuery::CombinedFields(query) => Arc::new(CombinedFieldsQueryExec::new( |
There was a problem hiding this comment.
CombinedFields bypasses the unindexed-fragment fallback, so a default full-text search silently misses rows appended after the indexes were built.
On this head, the following fails with match == [1] and combined == []. Running optimize_indices() first makes the combined query return [1], which isolates the problem to index coverage.
Reproducer (`cd python && uv run python`)
import tempfile
import lance
import pyarrow as pa
from lance.query import CombinedFieldsQuery, FullTextOperator, MatchQuery
uri = tempfile.mkdtemp(prefix="pr7905-unindexed-")
ds = lance.write_dataset(pa.table({"id": [0], "title": ["old"], "body": ["content"]}), uri)
ds.create_scalar_index("title", "INVERTED")
ds.create_scalar_index("body", "INVERTED")
ds = lance.write_dataset(
pa.table({"id": [1], "title": ["alpha"], "body": ["omega"]}),
uri,
mode="append",
)
combined = ds.to_table(
columns=["id"],
full_text_query=CombinedFieldsQuery(
"alpha omega", ["title", "body"], operator=FullTextOperator.AND
),
)["id"].to_pylist()
match = ds.to_table(
columns=["id"], full_text_query=MatchQuery("alpha", "title")
)["id"].to_pylist()
assert match == [1]
assert combined == [1] # actual: []The existing MatchQuery planner computes unindexed_fragments and unions a flat plan, while this arm always creates an index-only exec. Columns indexed at different dataset versions therefore also get incomplete BM25F membership and statistics.
| let mut column_total_tokens = 0u64; | ||
| let mut column_doc_freq = vec![0usize; terms.len()]; | ||
| for index in &column.indices { | ||
| let (total_tokens, num_docs, token_docs) = index.bm25_stats_for_terms(&terms).await?; |
There was a problem hiding this comment.
Legacy V1/V2 List<String> indexes mix two document domains here: bm25_stats_for_terms returns element-level docCount / docFreq, while the fallback later merges postings and document lengths by row ID. This changes top-k results for an old index versus rebuilding the same data on this head.
Reproducer
Create a venv containing pylance==8.0.0, then generate a V1 index:
export REPRO_URI="$(mktemp -d)/old.lance"
LANCE_FTS_FORMAT_VERSION=1 /tmp/lance8/bin/python - <<"PY"
import os
import lance
import pyarrow as pa
title = [["alpha"] * 10, ["beta"]] + [["gamma"]] * 8
body = [["zzz"]] * 10
ds = lance.write_dataset(
pa.table({"id": range(10), "title": title, "body": body}),
os.environ["REPRO_URI"],
)
ds.create_scalar_index("title", "INVERTED")
ds.create_scalar_index("body", "INVERTED")
PYQuery that index from this PR head:
import os
import lance
from lance.query import CombinedFieldsQuery
ds = lance.dataset(os.environ["REPRO_URI"])
out = ds.to_table(
columns=["id", "_score"],
full_text_query=CombinedFieldsQuery("alpha beta", ["title", "body"]),
limit=1,
)
assert out["id"].to_pylist() == [0] # actual: [1]The old V1 and V2 indexes both return id=1, score=2.2984569; rebuilding identical data on this head returns id=0, score=3.1963050.
| let mut boundary = 0; | ||
| while boundary < num_terms { | ||
| let bound = cursors[order[boundary]].upper_bound(); | ||
| if cumulative + bound <= threshold { |
There was a problem hiding this comment.
This f32 ceiling is not conservatively rounded, so MAXSCORE can classify the only term as non-essential and stop before a strictly higher-scoring document.
A one-term, limit=1 counterexample with valid u32 frequencies and lengths is:
let avgdl = ((3_324_876_276u64 + 2_691_694_489u64) as f64 / 2.0) as f32;
let idf = ((2.0f32 - 2.0 + 0.5) / (2.0 + 0.5) + 1.0).ln();
let bound = idf * (1.2 + 1.0);
let score = |tf: u32, dl: u32| {
let norm = 1.2 * (1.0 - 0.75 + 0.75 * dl as f32 / avgdl);
idf * ((1.2 + 1.0) * tf as f32 / (tf as f32 + norm))
};
let first = score(91_135_840, 3_324_876_276);
let better = score(1_957_490_862, 2_691_694_489);
assert_eq!(bound.to_bits(), first.to_bits());
assert!(better > bound); // one ULP higherIf the first row ID is smaller, it sets threshold == bound; the condition here then removes the only essential cursor and the loop exits without evaluating better. The finite-score premise also fails for accepted inputs: try_with_boosts accepts f32::MAX, and a two-token weighted length overflows to Inf, producing a NaN score.
| // Open every target column's segments and pair each with its boost. | ||
| let mut columns = Vec::with_capacity(query.columns.len()); | ||
| let mut all_segments = Vec::new(); | ||
| for (column, &weight) in query.columns.iter().zip(&query.boosts) { |
There was a problem hiding this comment.
The execution boundary silently changes the query when the public columns and boosts vectors are out of sync. A safe Rust caller can construct a validated two-column query and then call query.boosts.pop(); this zip searches only the first column with no error even though query.columns still names both. Direct struct construction can likewise bypass the duplicate, finite, and minimum-weight checks. This makes the public query contract depend on callers never using operations the type permits.
| MIN_OK="${MIN_OK:-0.95}" | ||
| LUCENE_DIR="${LUCENE_DIR:-$HOME/repos/extern/lucene}" | ||
| WORK="${WORK:-${TMPDIR:-/tmp}/combined_fields_compare}" | ||
| rm -rf "$WORK"; mkdir -p "$WORK" |
There was a problem hiding this comment.
WORK is caller-controlled but is recursively deleted before any tool or path validation. Pointing it at an existing directory destroys that directory; for example, setting it to the user home directory would erase the home directory. Quoting prevents word splitting, but it does not constrain the deletion target.
| }; | ||
|
|
||
| pre_filter.wait_for_ready().await?; | ||
| let (doc_ids, scores) = combined_fields_search( |
There was a problem hiding this comment.
combined_fields_search performs the complete synchronous materialization and MAXSCORE phase inside this stream::once async future. If any source is legacy, unsorted, or plain, the global fallback builds and sorts per-term HashMaps and then runs the full scoring loop without an await or CPU-pool boundary. A large query therefore occupies a DataFusion/Tokio worker and cannot respond to stream drop or task cancellation until the whole CPU section returns; the existing single-column path offloads its analogous bm25_search work.
e31eeda to
c1a86a8
Compare
|
Thanks for reviewing @Xuanwo There's one issue I left out, because it might be done better in a follow-up to keep the scope contained: Mixed-coverage scores are approximate
A single This is not a regression. The single-column path already behaves this way:
Fixing it requires one scorer shared by both children, so the blended statistics must exist before either child runs. It should cover the single-column path at the same time. |
Additional follow-up fixesA bunch of issues that were surfaced while fixing the review remarks. Wrong resultsStale data ignored after an overlay (36a94a1): with a data overlay, a Duplicate rows under stable row ids: the exclusion that keeps a row from being scored twice was built on row addresses, but the index stores logical ids when stable row ids are on, so it silently matched nothing. One row came back twice with two different scores. Wrong or unstable ordering
Tied scores shuffled between runs: no Errors and crashesMulti-column JSON queries crashed (a089612): a stream reported it carried one column while actually emitting all of them, so looking up the second column failed outright. Same bug could also make the single-column path silently read the wrong column.
PerformanceMemory grew with column count (9d37e3f): the flat path buffered a dense per-row, per-column, per-term table plus a full second copy. Now ~5–10× smaller and flat in the column count. Read pruning quietly stopped working (3314bca): after any compaction, the check that enables block skipping always failed, disabling the feature's headline optimisation. Invisible: results stayed correct, no test failed. Also: Cache statistics undercounted (1a826bf): Test integrityA test that guaranteed nothing (c4feb0b): the test asserting the fast and slow paths agree bit-for-bit had drifted onto a code path production never uses. Breaking the real path left it green. Now runs against both. Coverage gaps closed (0d16c87): JSON, nulls, list columns, filters, deletions, nested columns, and three-column cases were all unexercised. The reference implementation also had to be corrected first. Went from 18 to 31 tests. |
|
@Xuanwo this should now be ready for a second round of review. |
1a826bf to
8207934
Compare
|
8207934 2f8f59a adapt to the changes in #8073 as that's merged now. @BubbleCal |
8207934 to
0954b73
Compare
0954b73 to
5726c67
Compare
|
Adapt to the latest changes on main: Document granularity (#7788)BM25F joins target columns on the row address and sums per-row Two issues once element coordinates exist:
Scoring on mixed plansPre-existing, not from the rebase. The indexed child built its scorer from index statistics alone, the flat child from index statistics plus its
Also
Behaviour note: top-level |
|
@Xuanwo let me know if I can do anything to make this easier to review. |
5726c67 to
0a464e3
Compare
lance-gatekeeper
Bot
added
the
K-changes
35aeca4 to
e4446fd
Compare
lance-gatekeeper
Bot
removed
the
K-changes
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
K-changes
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
K-approved
lance-gatekeeper
Bot
added
K-approved
Score several text columns as one virtual field (Lucene's `CombinedFieldQuery` / BM25F blend) instead of the per-field max fusion `MultiMatch` does. Adds the query type with serde and JSON parsing, the `CombinedFieldsBM25Scorer`, the indexed scan and the planner and execution nodes that drive it. BM25F blends per-column term frequencies and document lengths into one `tf'`/`dl'` per row, so the scan is row-granular by construction. Two consequences shape the design: Row granularity, not document granularity. An inverted index may hold one document per list element and report `_doc_index` coordinates. BM25F cannot use such an index: it joins the target columns on the row address, and element coordinates of different columns have no correspondence to pair them on. `combined_fields` therefore declares itself row-granular everywhere the granularity plumbing asks, and rejects a target column that can only supply element documents. Corpus statistics must match that granularity. Releases before lance-format#7656 indexed each `List<String>` element as its own document, so those files report element-scoped `docCount`/`docFreq` while the scan accumulates by row. Mixing the two domains corrupts `idf'` and `avgdl'`, shifting an old index's top-k relative to the same data reindexed on a current build. Hence `bm25_row_stats_for_terms`, which counts distinct rows, delegating to the document-granular path on V3 where one row owns one document. A cross-field score is complete only when every target column's index holds the row, because `dl'` sums each column's length and a row absent from a column's `DocSet` contributes 0. This commit therefore requires every target column to cover every scanned fragment and refuses the query otherwise, naming the uncovered fragments and the columns to reindex. Scoring the rows no index covers is the next commit. The indexed scan reads every posting up front, then scores the union of the query terms' postings and keeps a bounded top-k. Every candidate is scored, so the result is exact by construction, and candidates are visited in ascending row-id order, which makes the top-k deterministic under ties. MAXSCORE pruning and read pruning are both follow-ups.
Dataset-level coverage for BM25F, checked against an independent brute-force BM25F reference (`lance_index::scalar::inverted::oracle`) that re-derives every statistic from the raw text, so it shares no code with the scan it checks. Each case asserts exact scores rather than just a hit set, because a wrong corpus size still returns the right rows in almost the right order. That is what pins down the parts easy to get subtly wrong: the per-column `w_f` factors, which are invisible at unit weights; ties, where the score-then-row ordering has to be deterministic across runs; and top-k across every k, where the pruning must agree with an exhaustive scan. Also covers the released-format fixtures (V1 and V2) so the row-granularity statistics path runs against real files rather than synthetic ones, nulls and empty strings, and the refusal paths: no index on any target column, and `fast_search` without full coverage.
…d_fields The previous commit refuses a `combined_fields` query whose target columns do not all cover every scanned fragment, so a default full-text search fails on any dataset with rows appended since the indexes were built. `MatchQuery` already unions in a flat scan for its unindexed fragments; this does the same for BM25F. Coverage is per column here, which makes it more than a copy of the single-column path. `dl'` sums each column's document length and a row absent from a column's `DocSet` contributes 0, so a fragment indexed for `title` but not `body` cannot be scored from the index at all. The indexed scan is therefore restricted to the intersection of per-column coverage and everything else goes to the flat scan, rather than splitting on the union. Both sides then score against one shared corpus. The flat side alone sees the rows no index covers, so it measures their contribution and publishes the blend; the indexed side waits for it instead of folding only its own `docCount'`/`docFreq'`/`avgdl'`. Without that, a row reached through either path would rank differently depending on which side happened to score it. Data overlays are handled by measuring rather than patching. When a target column carries an overlay-stale index entry, folding the flat row into the index statistics would double count it against the entry it replaces, and the flat scan cannot subtract what it replaced. So the corpus is measured from current data instead: every target fragment is scanned, every row folded into every column, and the index statistics left out. That costs a full scan of the target columns, so it stays confined to the stale case. `fast_search` is unchanged, being index-only by contract.
…tistics The cases that matter here are the ones where a shared corpus is easy to lose, since both scan sides must agree on `docCount'`/`docFreq'`/`avgdl'`: unindexed and partially indexed fragments, per-column index skew over both row-id schemes, a mixed indexed/flat plan, deletions followed by optimize, and overlay-stale fragments. Each asserts exact scores against the brute-force reference, because scoring the two sides against different corpora still returns the right rows in almost the right order. Also covers what only the flat path reaches: nulls and empty strings read from the scan rather than an index, list and nested columns, a column under a list, filters, and the plan shape itself, so a query that should union does not silently answer from the index alone. The external row-address prefilter is covered for the same reason, over both plan shapes: the fully covered plan ANDs the mask into the indexed scan's prefilter, while the mixed plan's flat child never reaches an index-side prefilter and is masked on its output instead. Scores are asserted there too, because the mask picks what is emitted and not the corpus it is measured against, so a surviving row must keep the score the unmasked query gave it.
e4446fd to
b9e2ba5
Compare
lance-gatekeeper
Bot
removed
K-approved
There was a problem hiding this comment.
The rebase preserves the reviewed combined-fields mechanism and keeps its row-statistics path independent of the newly merged prepared-scorer optimization.
The author accepts that coverage-triggered filtered plans can change scores and top results across optimize_indices(), and accepts the same trade-off for fragment selection. The overlay-statistics approximation can also double-count patched rows and vary with fragment selection. No further change is requested for these accepted risks, and no independent blocker remains.
lance-gatekeeper
Bot
added
K-approved
2 participants
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.This PR is the feature and its correctness: BM25F scoring over indexed data, plus a flat scan so a query still works on rows appended since the indexes were built. It is verified against an independent BM25F oracle that shares no code with the scan it checks.
Performance work is stacked on top in separate branches, not in this PR. See Stack. Language bindings are also separate: #8549 (Java) and #8550 (Python).
@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 this PR
b14f48c9c18a58b1736ef87509fb9e2ba5bcThe first half as a standalone PR on its own would ship a
combined_fieldsthat errors on any dataset with appended rows. So it is split into commits instead, alternating production and test code:About 60% of the diff is tests (~5,500 lines against ~3,600 of production code).
How it works
BM25F blend (per query term
t, fieldsfwith weightsw_f; LuceneCombinedFieldQuery):Execution flow:
Design consequences:
Row granularity, not document granularity. BM25F joins the target columns on the row address, so element coordinates from different columns have no correspondence to pair them on.
combined_fieldsdeclares itself row-granular everywhere the granularity plumbing asks and rejects a target column that can only supply element documents.Corpus statistics must match that granularity. Releases before #7656 indexed each
List<String>element as its own document, so those files report element-scopeddocCount/docFreqwhile the scan accumulates by row. Mixing the two domains corruptsidf'andavgdl'. Hencebm25_row_stats_for_terms, which counts distinct rows and delegates to the document-granular path on V3, where one row owns one document.One shared corpus across both children. Only the flat side sees the rows no index covers, so it measures their contribution and publishes the blend; the indexed side waits for it rather than folding only its own
docCount'/docFreq'/avgdl'. Without that, a row reached through either path would rank differently depending on which side happened to score it.Correctness
scalar::inverted: 589/589. Includes the row-granularity statistics path against real released V1 and V2 index files, not synthetic fixtures.combined_fieldsdataset tests: 28 test functions / 39 cases inrust/lance/src/dataset/tests/dataset_fts_combined_fields.rs, plus overlay coverage indataset_overlay_index_masking.rs. 60 tests match thecombinedfilter acrosslance --lib.lance_index::scalar::inverted::oracle, an independent brute-force BM25F reference that re-derives every statistic from the raw text. A wrong corpus size still returns the right rows in almost the right order, so hit-set assertions would not catch it.fast_searchrefusal paths, and the plan shape itself — so a query that should union cannot silently answer from the index alone.(score DESC, row_id ASC)viaRankedDoc, so both which rows survive a tie at the k-th score and the order they come back in are fixed by the data. That has to be settled in the search because a fully covered dataset returns the exec directly with noSortExecabove it; mixed plans keep the same ordering as their sort's second key. Pinned bytest_fts_combined_fields_tied_scores_are_deterministicacross both plan shapes.Performance
For reviewing this PR: of the three perf layers, only the
dl'length lookup (DocSet::doc_length_by_row_id, commit 1) is here. It removes the O(total-docs) length pass, which is the dominant cost on uniform-df workloads. MAXSCORE scoring pruning and posting-block read pruning are in the stacked branches below, socombined_fieldsin this PR is expected to be slower thanbest_fieldson skewed workloads until those land.To reproduce once the bench branch is checked out:
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.shStack
combined-fields-bm25fJsonTextStreamcombined-fields-pythoncombined_fieldscombined-fields-javacombined_fieldsThe four perf/bench branches have no PRs yet. I wanted to agree the split first. Because these are fork branches, GitHub can't stack the PRs on each other, so #8549 and #8550 currently show this PR's commits too; their own diff is the compare link above.