fix(fts): re-evaluate overlay-stale rows without segment scans - #7975
fix(fts): re-evaluate overlay-stale rows without segment scans#7975Xuanwo wants to merge 11 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughChangesFTS phrase and overlay execution
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Scanner
participant fts_overlay_plan
participant PhraseQueryExec
participant FlatMatchQueryExec
participant flat_phrase_search_stream_with_metrics
Scanner->>fts_overlay_plan: compute stale rows and overlay mode
fts_overlay_plan->>PhraseQueryExec: apply overlay block to indexed execution
fts_overlay_plan->>FlatMatchQueryExec: provide stale and unindexed rows
FlatMatchQueryExec->>flat_phrase_search_stream_with_metrics: evaluate phrase slop
Scanner->>Scanner: combine indexed and flat results
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
A-index
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
🟡 Other comments (1)
rust/lance/src/dataset/scanner.rs-3389-3391 (1)
3389-3391: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComment describes the old fragment-level behavior. This PR moves overlay handling to row granularity: stale rows are masked out of the index result and re-evaluated through a targeted take, not "drop stale segments" and re-scan "affected fragments." Please update the wording to match the row-level semantics.
📝 Suggested rewording
- // Data overlay masking: match and phrase queries drop stale segments and re-evaluate the - // affected fragments on their flat-text paths. This keeps stale index hits out while - // preserving current-value matches unless `fast_search` explicitly skips flat evaluation. + // Data overlay masking: match and phrase queries block overlay-stale rows from the indexed + // result and re-evaluate just those rows (plus any unindexed fragments) on the flat-text + // path. This keeps stale index hits out while preserving current-value matches unless + // `fast_search` explicitly skips flat evaluation.🤖 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/scanner.rs` around lines 3389 - 3391, Update the data overlay masking comment near the scanner logic to describe row-level behavior: stale rows are masked from index results and re-evaluated through a targeted take, while current-value matches remain preserved unless fast_search skips flat evaluation. Remove references to stale segments and affected-fragment rescans.Source: Coding guidelines
🧹 Nitpick comments (1)
rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs (1)
793-892: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParameterize the new row-level masking/targeted-take tests over
stable_row_ids.
test_fts_overlay_row_level_masking_under_fast_searchandtest_fts_overlay_flat_path_takes_only_stale_rowsvalidate the core row-level re-evaluation feature introduced by this PR, but both hardcodecreate_text_dataset(false). The sibling stale-drop tests were upgraded to#[rstest]+#[values(false, true)]specifically because overlay masking is expected to behave consistently across stable vs. non-stable row-id modes; these two new tests should get the same treatment to catch row-addressing regressions specific toenable_stable_row_ids.♻️ Suggested parameterization
-#[tokio::test] -async fn test_fts_overlay_row_level_masking_under_fast_search() { - let mut dataset = create_text_dataset(false).await; +#[rstest] +#[tokio::test] +async fn test_fts_overlay_row_level_masking_under_fast_search(#[values(false, true)] stable_row_ids: bool) { + let mut dataset = create_text_dataset(stable_row_ids).await;Apply the same change to
test_fts_overlay_flat_path_takes_only_stale_rows.Based on learnings, the graph context notes that the
stable_row_idstest parameter exists precisely so overlay masking/stale-row behavior can be verified consistent across both row-id modes; that rationale applies equally to these two new 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_overlay_index_masking.rs` around lines 793 - 892, Parameterize both test_fts_overlay_row_level_masking_under_fast_search and test_fts_overlay_flat_path_takes_only_stale_rows with rstest and values(false, true) for stable_row_ids. Accept the parameter in each test and pass it to create_text_dataset instead of hardcoding false, preserving all existing assertions and test behavior.
🤖 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.
Other comments:
In `@rust/lance/src/dataset/scanner.rs`:
- Around line 3389-3391: Update the data overlay masking comment near the
scanner logic to describe row-level behavior: stale rows are masked from index
results and re-evaluated through a targeted take, while current-value matches
remain preserved unless fast_search skips flat evaluation. Remove references to
stale segments and affected-fragment rescans.
---
Nitpick comments:
In `@rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs`:
- Around line 793-892: Parameterize both
test_fts_overlay_row_level_masking_under_fast_search and
test_fts_overlay_flat_path_takes_only_stale_rows with rstest and values(false,
true) for stable_row_ids. Accept the parameter in each test and pass it to
create_text_dataset instead of hardcoding false, preserving all existing
assertions and test behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 77225985-da5e-4cfb-8fa3-2e7e143f69f7
📒 Files selected for processing (6)
rust/lance-index/src/scalar/inverted/index.rsrust/lance/src/dataset/overlay.rsrust/lance/src/dataset/scanner.rsrust/lance/src/dataset/tests/dataset_overlay_index_masking.rsrust/lance/src/io/exec/fts.rsrust/lance/src/io/exec/utils.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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 (2)
rust/lance/src/dataset/scanner.rs (2)
3562-3677: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSubstantial duplicated overlay-planning logic between
plan_phrase_queryandplan_match_query.These two functions replicate the same sequence almost line-for-line: computing
target_fragments/unindexed_fragments, the "all-unindexed + fast_search" early return, callingfts_overlay_planand destructuringFtsOverlayPlan, building the overlay block mask, applying it to the indexed exec, and deciding whether to build a flat plan. The only real difference is the query/exec type (PhraseQuery/PhraseQueryExecvsMatchQuery/MatchQueryExec).Because this duplicated block encodes the overlay-masking safety invariant (blocking stale rows from the indexed path and re-evaluating them via the flat path), a future fix or edge case addressed in one function but missed in the other would silently reintroduce stale-row leakage for one of the two query types. Consider factoring the shared planning steps (fragment partitioning,
fts_overlay_planresolution, overlay-block computation) into a helper that returns the pieces needed by each caller to build its specific*QueryExec.♻️ Sketch of shared extraction
struct FtsPlanContext { target_fragments: Vec<Fragment>, unindexed_fragments: Vec<Fragment>, stale_rows: HashMap<u32, RoaringBitmap>, preset_segments: Option<Vec<IndexMetadata>>, overlay_block: Option<RowAddrMask>, } async fn resolve_fts_plan_context( &self, column: &str, index_name: &str, ) -> Result<Option<FtsPlanContext>> { // shared target_fragments / unindexed_fragments / fts_overlay_plan / overlay_block logic // returns None to signal the "return early" cases (all-unindexed, FullScan) are handled by caller }Also applies to: 3679-3807
🤖 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/scanner.rs` around lines 3562 - 3677, Extract the duplicated fragment partitioning, all-unindexed/FullScan handling, FtsOverlayPlan resolution, stale-row mask construction, and flat-plan eligibility logic from plan_phrase_query and plan_match_query into a shared helper or context type. Have both callers reuse that shared result while retaining their query-specific PhraseQueryExec and MatchQueryExec construction, including identical early-return behavior and overlay masking.
3894-3933: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winRemove the duplicate refine filter from
plan_flat_fts_input.In both the new filtered-read path and the prefilter legacy
scan_fragmentspath, the refine predicate is already applied byfiltered_read; wrappingfilter_plan.refine_expragain runs the same filter twice. Use an unfiltered flat fragment/stale input here, matching the existing KNN flat-fallback pattern.🤖 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/scanner.rs` around lines 3894 - 3933, Remove the explicit LanceFilterExec wrapping filter_plan.refine_expr in plan_flat_fts_input, since filtered_read already applies the refine predicate. Update both the filtered-read and legacy scan_fragments paths to use unfiltered flat fragment/stale inputs, matching the existing KNN flat-fallback pattern, while preserving the projection and alias behavior.
🧹 Nitpick comments (1)
rust/lance/src/dataset/overlay.rs (1)
141-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused fragment-level stale-overlay API.
collect_overlay_stale_fragshas no non-test callers outsiderust/lance/src/dataset/overlay.rs, while production planning now usescollect_overlay_stale_rows_for_segment. Keep the row-level variant and its tests; delete the fragment-level sibling.🤖 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/overlay.rs` around lines 141 - 159, Remove the unused collect_overlay_stale_frags function from overlay.rs, including any production-only imports or references made unnecessary by its deletion. Preserve collect_overlay_stale_rows_for_segment and all tests for the row-level behavior.
🤖 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/scanner.rs`:
- Around line 3562-3677: Extract the duplicated fragment partitioning,
all-unindexed/FullScan handling, FtsOverlayPlan resolution, stale-row mask
construction, and flat-plan eligibility logic from plan_phrase_query and
plan_match_query into a shared helper or context type. Have both callers reuse
that shared result while retaining their query-specific PhraseQueryExec and
MatchQueryExec construction, including identical early-return behavior and
overlay masking.
- Around line 3894-3933: Remove the explicit LanceFilterExec wrapping
filter_plan.refine_expr in plan_flat_fts_input, since filtered_read already
applies the refine predicate. Update both the filtered-read and legacy
scan_fragments paths to use unfiltered flat fragment/stale inputs, matching the
existing KNN flat-fallback pattern, while preserving the projection and alias
behavior.
---
Nitpick comments:
In `@rust/lance/src/dataset/overlay.rs`:
- Around line 141-159: Remove the unused collect_overlay_stale_frags function
from overlay.rs, including any production-only imports or references made
unnecessary by its deletion. Preserve collect_overlay_stale_rows_for_segment and
all tests for the row-level behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 10b5e9bd-d303-4a58-9160-e38cf7e4f587
📒 Files selected for processing (2)
rust/lance/src/dataset/overlay.rsrust/lance/src/dataset/scanner.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/src/dataset/overlay.rs`:
- Around line 135-136: Restore complete rustdoc for the public
collect_overlay_stale_rows_for_segment function: retain the behavior
description, add intra-doc links to the relevant overlay/segment structs and
methods, and include a compiling # Examples section using its current signature
and types. Keep the example synchronized with the implementation, or make the
function non-public if it is not intended to be part of the public API.
🪄 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: 565b8a32-60f4-4867-802f-4e6d0496eb0e
📒 Files selected for processing (1)
rust/lance/src/dataset/overlay.rs
| /// Compute exactly which row offsets within each covered fragment are stale and accumulate them | ||
| /// into `stale` (fragment_id → stale row offsets). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Restore complete documentation for the public API.
This comment documents public collect_overlay_stale_rows_for_segment, but the rewrite removed the required compiling example and links to relevant structs/methods. Add a synchronized # Examples section and intra-doc links, or reduce the function’s visibility if it is not intended as public API.
As per coding guidelines: “Document all public APIs with examples and links to relevant structs and methods; keep examples synchronized with actual signatures.”
🤖 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/overlay.rs` around lines 135 - 136, Restore complete
rustdoc for the public collect_overlay_stale_rows_for_segment function: retain
the behavior description, add intra-doc links to the relevant overlay/segment
structs and methods, and include a compiling # Examples section using its
current signature and types. Keep the example synchronized with the
implementation, or make the function non-public if it is not intended to be part
of the public API.
Source: Coding guidelines
|
FYI this tries to solve the same thing as #7952, so we should probably only merge one. I'll look at the difference and see which is a better direction. |
…w-level-bench-20260724 # Conflicts: # .github/workflows/rust.yml # rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs # rust/lance/src/io/exec/fts.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
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 (1)
rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs (1)
690-690: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winPass the new
stable_row_idsargument at every call site.Line 600 makes
create_text_datasetrequire a boolean, buttest_ngram_optimize_preserves_overlay_stalenessandtest_ngram_remap_excludes_newer_overlay_fragmentsstill use the old zero-argument form. The test module will not compile.Proposed fix
- let mut dataset = create_text_dataset().await; + let mut dataset = create_text_dataset(false).await;Apply the same change at both call sites and check for any remaining zero-argument calls.
Also applies to: 783-783
🤖 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_overlay_index_masking.rs` at line 690, Update both create_text_dataset call sites in test_ngram_optimize_preserves_overlay_staleness and test_ngram_remap_excludes_newer_overlay_fragments to pass the required stable_row_ids boolean, matching the intended test configuration. Search the test module for any remaining zero-argument create_text_dataset calls and update them as well.
🤖 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_overlay_index_masking.rs`:
- Line 690: Update both create_text_dataset call sites in
test_ngram_optimize_preserves_overlay_staleness and
test_ngram_remap_excludes_newer_overlay_fragments to pass the required
stable_row_ids boolean, matching the intended test configuration. Search the
test module for any remaining zero-argument create_text_dataset calls and update
them as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 89200b4d-0609-4d22-afc2-fdb70ac41673
📒 Files selected for processing (4)
rust/lance-index/src/scalar/inverted/index.rsrust/lance/src/dataset/tests/dataset_overlay_index_masking.rsrust/lance/src/io/exec/fts.rsrust/lance/src/io/exec/utils.rs
There was a problem hiding this comment.
These seem like legitimate points from Claude:
-
!target_fragments.is_empty() guard changes empty-fragment-list behavior (scanner.rs:3599, 3710). With with_fragments(vec![]), the old flat_fragments.len() == target_fragments.len() was 0 == 0 → flat plan over zero fragments → empty result. Now the guard skips that, fts_overlay_plan returns Unchanged (vacuous all() on an empty iterator), and you build a MatchQueryExec over the whole index with PreFilterSource::None. That returns rows from fragments the caller excluded. Narrow, but it's a silent correctness change; worth an early if target_fragments.is_empty() { return EmptyExec }.
-
There is no test with unindexed fragments and overlay-stale rows simultaneously — the one case where plan_flat_fts_input returns a UnionExec (scanner.rs:3950). Note that unlike the three sibling union sites (3019, 4235, 4396) this one has no RepartitionExec(1); it's only safe because FlatMatchQueryExec::required_input_distribution() is SinglePartition and EnforceDistribution runs in create_plan. That's legitimate but implicit, and it's exactly the shape that breaks if someone reorders the optimizer rules. A test that appends an unindexed fragment, overlays a row in an indexed one, and asserts both surface would pin it down.
|
Thanks, fixed both in Explicit empty fragment selections now return The focused tests, the existing delete-all regression, fmt, and full Rust clippy pass on AWS. |
3 participants
Why
FTS indexes cover immutable source rows, but field overlays can replace values after the index is built. Treating any intersecting overlay as invalidating the whole segment means that updating one row can turn an indexed FTS query into a flat scan across every covered fragment.
This keeps modern FTS segments indexed, blocks only the exact stale row addresses from index results, and re-evaluates those rows through a targeted take. Legacy segments without row-level coverage retain the conservative full-scan fallback. Match and phrase queries use the same overlay-aware path, including flat phrase evaluation for stale or unindexed rows.
Benchmark
Measured at commit
6687aa1eaf4b6bfbc3c49e5646a9b86820700ebaon an AWS EC2c7i.4xlargeinstance (16 vCPU, 32 GiB), Amazon Linux 2023, using therelease-with-debugprofile. The dataset contains 1,000,000 rows in 10 fragments and one FTS segment. Each result is the mean of 10 measured queries after one warm-up query.The FTS query searches for one unaffected row in the same fragment as the stale row:
The same benchmark also checks the shared overlay path for scalar and vector indexes:
cargo test -p lance --lib --profile release-with-debug -- overlay_index_masking::bench --ignored --nocapture --test-threads=1These are same-head overlay-overhead measurements, not a cross-version speedup claim. A plan-level regression test separately asserts that stale FTS rows are re-evaluated through a targeted take instead of a fragment scan.
At the same head, the focused AWS validation passed all 27 non-benchmark overlay/index tests and both FTS segment-selection tests.
Current head
ad7fa79dc920e6df1e97ba8c2cd6fc0a2497c472only adds the explicit empty-fragment short circuit and review-requested regression tests, so it does not affect the benchmarked non-empty query path. On the same AWS host, the current head passed both new focused tests, the existing delete-all inverted-index regression,cargo fmt --all -- --check, andcargo clippy --all --tests --benches -- -D warnings.Most changes in this PR are tests.