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
removed
the
K-risk
![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-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.
b9e2ba5 to
c09f32d
Compare
lance-gatekeeper
Bot
removed
K-approved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
K-approved
c09f32d to
30c7fc9
Compare
There was a problem hiding this comment.
The indexed-only split is acceptable: this PR computes exact BM25F for fully covered data and explicitly rejects incomplete coverage instead of returning partial or mis-scored results. #9444 is the verified follow-up that adds the flat scan; merge this indexed core first, then #9444 to make appended rows searchable without first optimizing the indexes.
Until then, ordinary combined_fields queries require full index coverage, MemWAL fresh-tier BM25F remains unsupported because it lacks corpus-wide statistics, and the indexed path exhaustively scores candidates pending pruning follow-ups. These are visible adoption constraints; focused current-head tests found no independent blocker.
|
Split into two PRs to keep the review manageable down to 5k loc (3k prod and 2k test code).
@Xuanwo this is now trimmed down and split as far as I could get. I would appreciate another review. |
BubbleCal
left a comment
BubbleCal
left a comment
There was a problem hiding this comment.
Thanks for this work!
This PR looks good to me, there's a problem:
lance now has index segments, each segment is an index covering some fragments. CombinedQuery would require that the given 2 columns' FTS index to exist on the same node, it's OK for single machine, but would be a problem in distributed nodes.
I will come back tmr to try to figure out how we can make the distributed search work, also welcome any thoughts!
An idea how it could work: Split distributed work by fragment set. Each worker loads all target columns' segments covering its fragments and scores only those fragments (with_covered_fragments). It uses statistics that are summed per column across workers and then blended into one BM25F scorer (with_base_scorer). The missing piece would be a way to pass the exec an explicit segment list per column, since it currently always opens all committed segments. Just an idea, I haven't really validated it yet. |
There was a problem hiding this comment.
The indexed-only BM25F split remains acceptable: fully covered queries use the cross-field scorer, while incomplete coverage fails explicitly. #9444 adds the flat path for appended rows; merge this indexed core first, then that follow-up.
The author’s distributed-execution sketch identifies a further adoption limit: this exec always opens every committed FTS segment for every target column. with_covered_fragments and with_base_scorer can restrict emitted rows and supply shared corpus statistics, but cannot select worker-local segments per column. The existing distributed BM25 plan explicitly excludes cross-column queries in V1, so distributed BM25F needs separate design and validation. No change is requested here for the single-node feature.
Until #9444 lands, ordinary queries need full index coverage. MemWAL fresh-tier BM25F remains unsupported, and the indexed path scores all candidates pending pruning work. These are adoption constraints, not blockers for this revision.
|
@BubbleCal if we'd extend the global statistics proposed in #8937 / #8938 to support multiple columns, we might be able to use that for distributed execution. Single nodeDistributed |
BubbleCal
left a comment
BubbleCal
left a comment
There was a problem hiding this comment.
I'm fine leaving distributed execution to a follow-up. We could let callers reuse the same explicit fragment grouping across column index builds, then co-locate the corresponding groups in the execution layer. Query execution should still fall back to reading the required segments from object storage when boundaries or placement differ. That makes alignment a locality optimization rather than a correctness requirement. Per-column segment selection and disjoint fragment scopes would give us a path forward without blocking this PR.
There was a problem hiding this comment.
The merge with current main preserves the reviewed BM25F design: target columns share a cross-field scorer, and normal scans reject fragments any target index cannot cover instead of returning partial results. #9444 is the open flat-scan follow-up for appended and overlay-stale rows; merge this indexed core first, then that path.
Until then, normal queries need full index coverage and fast_search deliberately skips uncovered fragments. The author accepted that fragment-scoped scores can change as coverage changes after optimize_indices(). MemWAL fresh-tier BM25F remains unsupported without corpus-wide field statistics, and the indexed path scores every candidate until pruning work lands.
The author's distributed design sketch remains a follow-up: this exec opens all committed segments, while #8937 excludes cross-column global statistics in V1. Worker-local BM25F needs per-column segment selection and shared statistics; this does not block the single-node feature.
There was a problem hiding this comment.
This test-only revision aligns nested-list coverage assertions with the indexed-only contract and uses refined vector distances for deterministic filter-order checks. It does not alter the reviewed BM25F scorer or planner.
Normal combined_fields queries still require every target index to cover each scanned fragment; fast_search skips uncovered fragments. #9444 remains the open flat-scan follow-up for appended and overlay-stale rows, so merge this indexed core first. The author accepted that fragment-scoped scores can change as coverage changes after optimize_indices(). MemWAL fresh-tier BM25F remains unsupported without corpus-wide statistics, and indexed search scores all candidates pending pruning work.
The author's distributed design sketch remains follow-up work: this exec opens all committed segments, while #8937 excludes cross-column global statistics in V1. Worker-local BM25F needs per-column segment selection and shared statistics; no change is requested here for the single-node feature.
|
Thanks @BubbleCal I was about to push a rebase to resolve the conflict but I saw you already merged main and updated in the meantime. I'll wait and update the downstream branches after this PR is merged. |
|
Thank you @sbrunk for the great work! Also @BubbleCal for the review. |
3 participants
TL;DR
Adds a
combined_fieldsfull text query that scores several text columns as one virtual field. This is BM25F, the same thing Elasticsearch callscombined_fieldsand Lucene callsCombinedFieldQuery. Today the only option isMultiMatch, which scores each column on its own and then keeps the best one.This PR covers the case where every target column is fully indexed. If a query reaches a fragment that one of the target columns has not indexed, it returns an error. Removing that restriction is the follow up PR, #9444, which adds a flat scan over those fragments. See Stack.
Language bindings are 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 several columns with
MultiMatchQuery, but each column is scored against its own statistics, and the results are combined by taking the highest score. That is what Elasticsearch callsbest_fields. There is no real cross field BM25. A term that is rare intitlebut common inbodyends up with two IDF values that cannot be compared to each other, and a query likejohninfirst_nameplussmithinlast_namecannot be scored as if both columns were one field. BM25F blends the statistics so the columns behave like a single field, with a weight per column.What is in this PR
9f289302c30c7fc939About 2,200 of the added lines are tests.
The whole public API is in this PR:
CombinedFieldsQuery, its JSON form in the FTS parser, tokenizer validation, and the row granularity rules. The follow up PR leaves all of it alone.query.rs,parser.rs,tokenizer.rsandtraits.rsare unchanged there. So the API review happens here, and the follow up is only about execution.How it works
For each query term
tand each columnfwith weightw_f, following Lucene'sCombinedFieldQuery:How a query runs:
The design has two consequences.
Scores are per row, not per list element. BM25F has to join the target columns on the row address. If two columns hold lists, their elements have no correspondence, so there is no way to pair them up.
combined_fieldsreports itself as row granular everywhere the granularity code asks, and rejects a target column that can only produce element documents.Corpus statistics have to use that same granularity. Releases before #7656 indexed each
List<String>element as its own document, so those index files reportdocCountanddocFreqper element, while this scan counts per row. Mixing the two produces wrongidf'andavgdl'values. That is whatbm25_row_stats_for_termsis for. It counts distinct rows, and on V3 it just delegates to the normal path, because there one row is one document.When the query returns an error
Coverage is decided per fragment rather than per row. A BM25F score needs
tf_fanddl_ffrom every target column, so a row has to be scored either entirely from the index or not at all. A fragment counts as not covered if any target column's index is missing it, or if a newer data overlay made its entries stale.If such a fragment is in scope, a normal scan fails with:
fast_searchis not affected. It only ever reads indexes, so it skips the fragments that are not covered and returns what the indexes have.The alternatives were to leave those rows out of the result, or to score them with statistics that do not describe the data. An error naming the columns to optimize seemed better than either. #9444 replaces it with a flat scan over exactly those fragments, using one shared set of corpus statistics for both sides.
Correctness
scalar::inverted: 850 of 850 tests pass. That includes the row granularity statistics path, checked against real released V1 and V2 index files rather than hand built fixtures.rust/lance/src/dataset/tests/dataset_fts_combined_fields.rs. 24 tests match thecombinedfilter inlance --lib.lance_index::scalar::inverted::oracle, a brute force BM25F reference that recomputes every statistic from the raw text and shares no code with the scan it checks. A wrong corpus size still returns almost the right rows in almost the right order, so a test that only checked which rows came back would pass anyway.fast_searchpaths that skip uncovered fragments. The plan shape is checked as well, so a query that should return an error cannot answer from partial statistics instead.RankedDoc. That decides both which rows survive a tie at the k-th score and the order they come back in. It has to happen inside the search, because when everything is covered the exec is returned directly with noSortExecabove it.Performance
There are three performance layers in this work, and only the
dl'lookup is in this PR (DocSet::doc_length_by_row_id). It removes a pass over all documents just to get their lengths, which is the main cost when term frequencies are fairly uniform. MAXSCORE pruning and posting block skipping are in the branches below, so on skewed datacombined_fieldswill be slower thanbest_fieldsuntil those land.Stack
combined-fields-bm25fJsonTextStreamcombined-fields-pythoncombined_fieldscombined-fields-javacombined_fieldsThe performance and bench branches do not have PRs yet. I wanted to agree the split first. These are all fork branches, and GitHub cannot stack PRs across forks, so each dependent PR also shows the commits of the ones below it. The compare links give the real diff for each.