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 6 commits into
Conversation
📝 WalkthroughWalkthroughThe change adds slop-aware flat FTS phrase matching and refactors FTS overlay handling to mask stale rows, re-evaluate affected data through flat paths, and combine indexed and flat results. Tests and benchmarks cover phrase behavior, overlays, fast-search handling, and stale-row rescoring. ChangesFTS phrase and overlay execution
Ancillary updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 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. |
2 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
a2ea2cc843ab2ba865ff0628fc0d1f2ad080e9e0on 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. Current headde342d898a018e3243d4b6fdc7b81501e21b7de8adds only cleanup and CI/build fixes after the measured commit: removal of an unused helper, initialization of an example-only ACORN flag, and a fixed QEMU version for CI. None of these changes affects the benchmarked execution path.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.
Most changes in this PR are tests.