fix(fts): block only overlay-stale rows from phrase queries#7952
Closed
wjones127 wants to merge 2 commits into
Closed
fix(fts): block only overlay-stale rows from phrase queries#7952wjones127 wants to merge 2 commits into
wjones127 wants to merge 2 commits into
Conversation
A phrase query excluded every FTS index segment that covered a fragment carrying an overlay on the indexed column. Phrase queries have no flat re-evaluation path, so those segments were dropped wholesale — taking with them correct matches that were merely co-located with the overlaid row and had nothing to do with the overlay. Overlaying one row could therefore erase every phrase hit in its segment. Block just the overlay-stale rows from the phrase result via the prefilter's `overlay_block` mask (mirroring the scalar and vector index paths) and keep every segment searchable. Stale hits are still removed; a new phrase match on a blocked row's current value still only surfaces after compaction folds the overlay into the base, as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPhrase FTS overlay handling now derives stale row masks and applies them during phrase execution, preserving fresh matches in affected segments. Prefilter and executor APIs support mask propagation, with an end-to-end test covering sibling matches. ChangesPhrase FTS overlay masking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Scanner
participant PhraseQueryExec
participant DatasetPreFilter
Scanner->>Scanner: derive stale row addresses
Scanner->>PhraseQueryExec: attach overlay block
PhraseQueryExec->>DatasetPreFilter: set_overlay_block
DatasetPreFilter-->>PhraseQueryExec: exclude stale phrase rows
PhraseQueryExec-->>Scanner: return fresh phrase matches
Possibly related PRs
Suggested labels: 🚥 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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/tests/dataset_overlay_index_masking.rs-818-820 (1)
818-820: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the documented fragment ranges.
With
max_rows_per_file: 6, fragment 0 contains ids 0..5 and fragment 1 contains ids 6..11; the current comment incorrectly includes ids 6 and 12.As per coding guidelines, “Ensure doc comments match actual semantics.”
🤖 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 818 - 820, Correct the documentation comment above the two-fragment text dataset test to state that fragment 0 contains ids 0..5 and fragment 1 contains ids 6..11, removing the incorrect references to ids 6 and 12 while preserving the rest of the dataset description.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 `@rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs`:
- Around line 822-855: The test fixture currently manually constructs an
ArrowSchema and RecordBatch. Replace this construction in the dataset overlay
masking test with the required record_batch!() or gen_batch() builder,
preserving the existing id/text columns and row values.
In `@rust/lance/src/io/exec/fts.rs`:
- Around line 1255-1259: The public overlay-mask APIs need linked, compiling
documentation examples. In rust/lance/src/io/exec/fts.rs lines 1255-1259, update
PhraseQueryExec::with_overlay_block documentation to link RowAddrMask and
demonstrate passing one into phrase-query prefiltering; in
rust/lance/src/index/prefilter.rs lines 241-245, update
DatasetPreFilter::with_overlay_block documentation to link the method and
demonstrate in-place setup before wait_for_ready. Keep both examples
synchronized with their actual signatures.
---
Other comments:
In `@rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs`:
- Around line 818-820: Correct the documentation comment above the two-fragment
text dataset test to state that fragment 0 contains ids 0..5 and fragment 1
contains ids 6..11, removing the incorrect references to ids 6 and 12 while
preserving the rest of the dataset description.
🪄 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: 2aa25a9e-4611-4404-9a73-54ba22c6ebf5
📒 Files selected for processing (4)
rust/lance/src/dataset/scanner.rsrust/lance/src/dataset/tests/dataset_overlay_index_masking.rsrust/lance/src/index/prefilter.rsrust/lance/src/io/exec/fts.rs
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Closing in favor of #7975 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
An FTS phrase query excluded every index segment that covered a fragment carrying a data overlay on the indexed column. Because phrase queries have no flat re-evaluation path (
plan_match_queryre-scores overlaid fragments flat;plan_phrase_querydid not), those segments were dropped wholesale — taking with them correct matches that were merely co-located with the overlaid row and had nothing to do with the overlay.Concretely, overlaying a single row's text could erase every phrase hit in that row's segment. Sequence that triggers it: write rows → merge-insert more → build FTS index (with positions) → overlay one non-matching row → a phrase query that should match sibling rows in the same segment returns nothing.
Fix
Block just the overlay-stale rows from the phrase result via the prefilter's
overlay_blockmask, and keep every segment searchable — mirroring the scalar (MaterializeIndexExec) and vector (ANNIvfSubIndexExec) index paths, which also block stale rows row-granularly rather than dropping whole segments.PhraseQueryExecgains anoverlay_block: Option<RowAddrMask>applied to its prefilter before search.plan_phrase_querycomputes the overlay-stale rows for the indexed column (collect_overlay_stale_rows_for_segment) and passes them as the block.DatasetPreFilter::set_overlay_blockmutates in place (theArc-held prefilter is finalized just beforewait_for_ready).Semantics are unchanged in the ways that matter: stale hits are still removed, and — as before — a new phrase match on a blocked row's current value only surfaces once compaction folds the overlay into the base (phrase has no flat path to re-score it). The only change is that non-overlaid sibling rows are no longer collateral damage.
Test
test_fts_phrase_overlay_keeps_sibling_matches: a fragment holds three phrase matches plus one non-matching row; overlaying only the non-matching row must not drop the three siblings (nor a fourth match in a second fragment). Fails before the fix (returns[]), passes after. All existing overlay-masking and FTS tests still pass.🤖 Generated with Claude Code