feat(index): mask data overlay files in scalar and vector index queries#7549
Conversation
Benchmark results — OSS-1325 overlay index maskingAll numbers are warm-cache, NVMe local disk, release build (Python benchmarks via 1. BTree — stale row count scalingDataset: 1M rows / 10 fragments (100k rows/frag). Query:
Row-level masking keeps overhead proportional to stale row count. At 10% stale density (10k stale rows in a 100k-row fragment), overhead is only +5ms on a 1M-row query. The fragment-level approach would have sent 10 × 100k rows to flat scan — orders of magnitude worse. 2. BTree — fragment coverageDataset: 1M rows / 10 frags. 1% stale rows per overlaid fragment.
~0.5 ms overhead per additionally-overlaid fragment. The fast-path (skip all stale analysis when no fragment has any overlay) is effective — the 0-overlay baseline is indistinguishable from a dataset with no overlay support. 3. BTree — overlay file count (compaction motivation)Dataset: 100k rows / 1 fragment. Each commit overlays the same 1% of rows, so stale row count stays constant while file count grows.
Key finding: overhead scales linearly with overlay commit count because each commit adds a 4. BTree — non-indexed field overheadDataset: 100k rows / 1 fragment. BTree on
Both overlay scenarios add ~1.1–1.3 ms. The current fast-path fires only when there are no overlays at all; when any overlay exists, we still load BTree segment metadata and iterate overlay field IDs before concluding 0 stale rows. A second fast-path that checks field IDs before loading segments would eliminate this overhead for non-indexed fields. Left as a follow-up optimization. 5. Vector ANN — recall and latencyDataset: 50k rows / 1 fragment, 32-dim IVF_FLAT (1 partition). 20 query vectors, k=10.
Perfect recall at 1% stale rate (500 blocked rows). No latency regression. As a point of reference from the Rust-level benchmark:
+0.8 ms for a single overlay targeting 1 row in a 100k-row fragment (vs. the old fragment-level approach which forced 100k-row flat KNN, adding ~1.9 ms). 6. FTS — stale fragment countDataset: ~105k rows / 11 fragments (10k rows/frag), realistic English sentences via
Result count is identical across all stale-frag counts — correctness is maintained. Even with 10 of 11 fragments stale (essentially pure flat text scan), overhead is under 1 ms. Fragment-level FTS masking is sufficient: the flat text scan on short English sentences is cheap, and a row-level FTS path would save at most ~1 ms in the worst case, not worth the complexity of a new scorer node. Summary
The main performance risk is overlay file accumulation without compaction (benchmark 3): 64 uncompacted commits cause a 5.7× slowdown even with the same stale row set. Compaction is important for production workloads with frequent updates. |
3ba338b to
a7d4508
Compare
0cc71e4 to
b0da3e2
Compare
a7cfcf0 to
72cc6d2
Compare
d40e7b4 to
6dd1099
Compare
4bb5e39 to
5ff87ce
Compare
6dd1099 to
3959345
Compare
|
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:
📝 WalkthroughWalkthroughPost-index overlays now identify stale indexed rows by version and indexed fields. Scalar, FTS, and vector scan paths mask stale index candidates and re-evaluate affected rows or fragments, with new execution plumbing and comprehensive integration tests. ChangesOverlay index masking
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Scanner
participant OverlayCollector
participant IndexExecution
participant FlatRecheck
Scanner->>OverlayCollector: collect stale rows and fragments
Scanner->>IndexExecution: apply overlay_block
IndexExecution-->>Scanner: return masked index results
Scanner->>FlatRecheck: re-evaluate stale rows or fragments
FlatRecheck-->>Scanner: return current-value matches
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
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)
4127-4232: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCombined ANN + fallback + stale rescoring logic looks correct; a few new bare
.unwrap()calls.The overall approach (ensure vector column present, build fallback/stale flat-KNN inputs projected to
knn_node's schema, union, re-runflat_knnover the union) correctly generalizes the pre-existing fallback-only combination to also handle stale rows.debug_assert!(q.metric_type.is_some())is the right call per guidelines.Two new bare
.unwrap()s worth tightening:
self.dataset.empty_projection().union_column(&q.column, OnMissing::Error).unwrap()(~4159-4160)self.dataset.schema().project(&columns).unwrap()(~4176)Both are unlikely to fail in practice (the columns always exist), but per coding guidelines this should propagate via
?instead.As per coding guidelines,
rust/**/*.rs: "Avoid bare.unwrap()... If unavoidable, use.expect(\"reason\")."🤖 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 4127 - 4232, Replace the bare unwraps in the vector projection setup and fallback scan projection within the surrounding scanner method with error propagation via ?. Preserve the existing projection construction and pass the resulting projections to take and scan_fragments, allowing both projection errors to return from the method.Source: Coding guidelines
2952-3009: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSound design for masking + re-evaluating stale rows on the indexed-scan path.
Computing
overlay_stale_rowsfrom the candidate fragments, blocking them viawith_overlay_block, and unioning a targeted stale-Take path back in is consistent with the documented design and withFilteredReadOptions's exposedwith_overlay_blockbuilder contract.One nit:
let filter = filter_plan.full_expr.as_ref().unwrap();(line 2992) is a bare.unwrap()on anOption. It's guarded implicitly byfilter_plan.index_query.is_some()a few lines up (index_query and full_expr are expected to be set together), but that invariant isn't enforced by the type system here. See the consolidated note on this pattern below.As per coding guidelines,
rust/**/*.rs: "Avoid bare.unwrap(); useif let,match,let ... else,?, or combinators... If unavoidable, use.expect("reason")."🤖 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 2952 - 3009, Replace the bare unwrap of filter_plan.full_expr in the stale-Take path with explicit handling of the missing expression, using the surrounding filter-plan flow to return or propagate an appropriate error before constructing the filtered stale plan. Preserve the existing behavior when full_expr is present and keep the index_query guard unchanged.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/overlay.rs`:
- Around line 1169-1252: Move the inline DataFile and DataOverlayFile imports
out of dense_overlay and test_exclusion_offsets_sparse_per_field, since they are
already available through the tests module scope. Keep only the necessary
OverlayCoverage import, placing it at the tests module’s top-level imports.
- Around line 89-155: Extract the shared version gate and overlay exclusion
calculation from collect_stale_overlay_frags and
collect_overlay_stale_rows_for_segment into a helper such as
stale_offsets_for_fragment, returning an empty RoaringBitmap when all overlays
predate segment.dataset_version and otherwise calling overlay_exclusion_offsets.
Update both callers to use this helper while preserving their existing
fragment-level and row-level stale accumulation behavior.
- Around line 89-155: Update collect_stale_overlay_frags and
collect_overlay_stale_rows_for_segment so a missing segment.fragment_bitmap is
treated as covering every fragment, matching DatasetPreFilter::new. Remove the
early return for None and make the coverage check skip only fragments explicitly
absent from Some(bitmap); preserve existing stale-fragment and stale-row masking
behavior for covered fragments.
In `@rust/lance/src/dataset/scanner.rs`:
- Around line 13750-14335: Add overlay masking test coverage using datasets
configured with enable_stable_row_ids: true, especially around
stale_rows_row_id_exec and RowId/RowAddr handling. Update the relevant dataset
builders or add focused scalar/vector/FTS cases while preserving the existing
assertions and ensuring stale rows are correctly excluded without corrupting row
identity.
- Around line 4560-4574: Replace the bare unwraps in the fallback projection
flow around filter_plan.full_expr and fallback_projection with explicit
invariant handling. In the fallback_projection construction, convert a missing
filter_plan.full_expr into an internal Error using the established error
pattern, and in the stale_rows path handle a missing fallback_projection without
panicking, preserving the existing behavior for valid states.
- Around line 4438-4476: Update stale_rows_row_id_exec and its callers so
physical RowAddress values are not emitted as _rowid inputs to TakeExec. Route
stale-row reads through the row-address-aware mechanism used by
u64s_as_take_input, including RowAddrTreeMap/RowAddrMask and FilteredReadExec
index_input, in new_filtered_read, scalar_indexed_scan, and knn_combined.
Preserve targeted stale-row re-evaluation and support datasets with stable row
IDs.
- Around line 4391-4436: The segment classification in
fts_stale_frags_and_fresh_segments must treat fragment_bitmap: None as unsafe.
Update the match so segments without a bitmap are excluded from fresh_segments
and routed through the flat path, or otherwise force the entire query to use the
flat path; preserve the existing handling for bitmap segments that do or do not
overlap stale_frag_ids.
- Line 5240: The exact-index shortcut around partition_frags_by_coverage must
account for stale_rows before returning ScalarIndexExec. Only use the direct
ScalarIndexExec path when stale_rows.is_empty(); otherwise fall back to the
existing filtered-read path, unless ScalarIndexExec already supports propagating
the overlay mask.
In `@rust/lance/src/io/exec/filtered_read.rs`:
- Around line 96-110: Update without_rows to enforce the documented block-list
invariant instead of silently skipping the update when block.block_list()
returns None. Assert that block_list() is present, then apply also_block to both
index_result.upper and index_result.lower; preserve the existing empty
block-list behavior.
In `@rust/lance/src/io/exec/knn.rs`:
- Around line 1109-1133: Replace the new
ANNIvfSubIndexExec::try_new_with_overlay constructor with the standard
constructor followed by a with_overlay_block builder call, passing the existing
optional overlay_block. Update ANNIvfSubIndexExec’s constructor/API accordingly
and adjust both call sites, including the path around new_knn_exec and the
additional occurrence noted by the review, without changing overlay behavior.
---
Outside diff comments:
In `@rust/lance/src/dataset/scanner.rs`:
- Around line 4127-4232: Replace the bare unwraps in the vector projection setup
and fallback scan projection within the surrounding scanner method with error
propagation via ?. Preserve the existing projection construction and pass the
resulting projections to take and scan_fragments, allowing both projection
errors to return from the method.
- Around line 2952-3009: Replace the bare unwrap of filter_plan.full_expr in the
stale-Take path with explicit handling of the missing expression, using the
surrounding filter-plan flow to return or propagate an appropriate error before
constructing the filtered stale plan. Preserve the existing behavior when
full_expr is present and keep the index_query guard unchanged.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 9922b37a-aacc-4439-92d1-09e75fa176c2
📒 Files selected for processing (7)
rust/lance/src/dataset/overlay.rsrust/lance/src/dataset/scanner.rsrust/lance/src/index/prefilter.rsrust/lance/src/index/vector/fixture_test.rsrust/lance/src/io/exec/filtered_read.rsrust/lance/src/io/exec/knn.rsrust/lance/src/io/exec/scalar_index.rs
854d400 to
031f086
Compare
| .unwrap(); | ||
| let write_params = WriteParams { | ||
| max_rows_per_file: 6, | ||
| max_rows_per_group: 6, |
There was a problem hiding this comment.
nitpick: groups don't exist past Lance 1.0 so we shouldn't bother setting this.
There was a problem hiding this comment.
Removed max_rows_per_group here and from the other three test datasets.
8a05c45 to
5c49fd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/mem_wal/scanner/point_lookup.rs`:
- Around line 138-148: Expand the rustdoc for the public builder methods
with_session and with_store_params to include compiling # Examples sections
using their current signatures, and add links to the related planner, Session,
and ObjectStoreParams APIs. Keep the examples focused on constructing the
builder and chaining each method, synchronized with the available types and
methods.
In `@rust/lance/src/dataset/overlay.rs`:
- Around line 53-68: Document the public overlay API functions in this module
with compiling doctest examples that match their current signatures. Add links
to relevant types and methods, including DataOverlayFile, Fragment, and
IndexMetadata, and cover the functions around overlay_exclusion_offsets through
the additionally referenced range. Keep examples minimal, valid, and
synchronized with the actual APIs.
In `@rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs`:
- Around line 556-591: The compound-index test currently uses identical
full-fragment coverage and does not exercise partial per-index coverage. Extend
test_overlay_stale_with_compound_index_expression by creating an append between
building the age and id indexes, then apply an overlay and query with a compound
AND filter that uses both indexed columns. Assert each matching row appears
exactly once, covering the path where one leaf uses new_data_path and the other
uses stale_take_path.
In `@rust/lance/src/index/prefilter.rs`:
- Around line 234-239: Use a block-list-specific representation for overlay
exclusions instead of accepting arbitrary RowAddrMask values. In
rust/lance/src/index/prefilter.rs:234-239, update Prefilter's with_overlay_block
to accept or validate RowAddrTreeMap, and at 398-400 combine only the validated
block mask. In rust/lance/src/io/exec/filtered_read.rs:100-109, remove the
release-mode silent no-op; at 1482-1487 store the block-list representation; at
1524-1529 validate the builder argument; and at 1906-1910 propagate validation
errors when applying the exclusion.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 9a957d04-36cc-4e27-8074-504e311a7ab3
📒 Files selected for processing (12)
rust/lance/benches/mem_wal/point_lookup/mem_wal_point_lookup_bench.rsrust/lance/src/dataset/index.rsrust/lance/src/dataset/mem_wal/scanner/point_lookup.rsrust/lance/src/dataset/overlay.rsrust/lance/src/dataset/scanner.rsrust/lance/src/dataset/tests/dataset_overlay_index_masking.rsrust/lance/src/dataset/tests/mod.rsrust/lance/src/index/prefilter.rsrust/lance/src/index/vector/fixture_test.rsrust/lance/src/io/exec/filtered_read.rsrust/lance/src/io/exec/knn.rsrust/lance/src/io/exec/scalar_index.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 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/mem_wal/scanner/point_lookup.rs`:
- Around line 138-148: Expand the rustdoc for the public builder methods
with_session and with_store_params to include compiling # Examples sections
using their current signatures, and add links to the related planner, Session,
and ObjectStoreParams APIs. Keep the examples focused on constructing the
builder and chaining each method, synchronized with the available types and
methods.
In `@rust/lance/src/dataset/overlay.rs`:
- Around line 53-68: Document the public overlay API functions in this module
with compiling doctest examples that match their current signatures. Add links
to relevant types and methods, including DataOverlayFile, Fragment, and
IndexMetadata, and cover the functions around overlay_exclusion_offsets through
the additionally referenced range. Keep examples minimal, valid, and
synchronized with the actual APIs.
In `@rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs`:
- Around line 556-591: The compound-index test currently uses identical
full-fragment coverage and does not exercise partial per-index coverage. Extend
test_overlay_stale_with_compound_index_expression by creating an append between
building the age and id indexes, then apply an overlay and query with a compound
AND filter that uses both indexed columns. Assert each matching row appears
exactly once, covering the path where one leaf uses new_data_path and the other
uses stale_take_path.
In `@rust/lance/src/index/prefilter.rs`:
- Around line 234-239: Use a block-list-specific representation for overlay
exclusions instead of accepting arbitrary RowAddrMask values. In
rust/lance/src/index/prefilter.rs:234-239, update Prefilter's with_overlay_block
to accept or validate RowAddrTreeMap, and at 398-400 combine only the validated
block mask. In rust/lance/src/io/exec/filtered_read.rs:100-109, remove the
release-mode silent no-op; at 1482-1487 store the block-list representation; at
1524-1529 validate the builder argument; and at 1906-1910 propagate validation
errors when applying the exclusion.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 9a957d04-36cc-4e27-8074-504e311a7ab3
📒 Files selected for processing (12)
rust/lance/benches/mem_wal/point_lookup/mem_wal_point_lookup_bench.rsrust/lance/src/dataset/index.rsrust/lance/src/dataset/mem_wal/scanner/point_lookup.rsrust/lance/src/dataset/overlay.rsrust/lance/src/dataset/scanner.rsrust/lance/src/dataset/tests/dataset_overlay_index_masking.rsrust/lance/src/dataset/tests/mod.rsrust/lance/src/index/prefilter.rsrust/lance/src/index/vector/fixture_test.rsrust/lance/src/io/exec/filtered_read.rsrust/lance/src/io/exec/knn.rsrust/lance/src/io/exec/scalar_index.rs
🛑 Comments failed to post (4)
rust/lance/src/dataset/mem_wal/scanner/point_lookup.rs (1)
138-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Document the new public builder methods with examples and links.
Add compiling
# Examplessections and links to the related planner, session, and store-parameter APIs, keeping examples synchronized with the current signatures.As per coding guidelines, Rust public APIs must be documented with examples and links to relevant structs and methods.
🤖 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/mem_wal/scanner/point_lookup.rs` around lines 138 - 148, Expand the rustdoc for the public builder methods with_session and with_store_params to include compiling # Examples sections using their current signatures, and add links to the related planner, Session, and ObjectStoreParams APIs. Keep the examples focused on constructing the builder and chaining each method, synchronized with the available types and methods.Source: Coding guidelines
rust/lance/src/dataset/overlay.rs (1)
53-68: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add compiling examples to the new public APIs.
These functions have semantic documentation but no usage examples. Include doctests and links to relevant types such as [
DataOverlayFile], [Fragment], and [IndexMetadata].As per coding guidelines, “Document all public APIs with examples and links to relevant structs and methods; keep examples synchronized with actual signatures.” <coding_guidelines>
Also applies to: 108-168
🤖 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 53 - 68, Document the public overlay API functions in this module with compiling doctest examples that match their current signatures. Add links to relevant types and methods, including DataOverlayFile, Fragment, and IndexMetadata, and cover the functions around overlay_exclusion_offsets through the additionally referenced range. Keep examples minimal, valid, and synchronized with the actual APIs.Source: Coding guidelines
rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs (1)
556-591: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift
Consider extending this compound-index test with differing per-index fragment coverage.
This test builds
ageandidindices that both cover the full fragment set, so it can't catch the partial-coverage duplicate-row scenario flagged onpartition_frags_by_coverageinscanner.rs(a fragment covered by only one of two compound-expression leaves, with an overlay on that leaf's column, could be emitted once vianew_data_pathand once viastale_take_path). Worth adding a case where one index is built before an append and the other after, then querying with a compoundANDfilter.🤖 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 556 - 591, The compound-index test currently uses identical full-fragment coverage and does not exercise partial per-index coverage. Extend test_overlay_stale_with_compound_index_expression by creating an append between building the age and id indexes, then apply an overlay and query with a compound AND filter that uses both indexed columns. Assert each matching row appears exactly once, covering the path where one leaf uses new_data_path and the other uses stale_take_path.rust/lance/src/index/prefilter.rs (1)
234-239: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use a block-list-specific type for overlay exclusions.
Both APIs accept arbitrary
RowAddrMaskvalues despite requiring block lists, producing inverted or silently skipped filtering for allow-list inputs.
rust/lance/src/index/prefilter.rs#L234-L239: acceptRowAddrTreeMapor reject non-block masks.rust/lance/src/index/prefilter.rs#L398-L400: combine only a validated block mask.rust/lance/src/io/exec/filtered_read.rs#L100-L109: remove the release-mode silent no-op.rust/lance/src/io/exec/filtered_read.rs#L1482-L1487: store a block-list-specific representation.rust/lance/src/io/exec/filtered_read.rs#L1524-L1529: validate the builder argument.rust/lance/src/io/exec/filtered_read.rs#L1906-L1910: propagate validation errors while applying the exclusion.📍 Affects 2 files
rust/lance/src/index/prefilter.rs#L234-L239(this comment)rust/lance/src/index/prefilter.rs#L398-L400rust/lance/src/io/exec/filtered_read.rs#L100-L109rust/lance/src/io/exec/filtered_read.rs#L1482-L1487rust/lance/src/io/exec/filtered_read.rs#L1524-L1529rust/lance/src/io/exec/filtered_read.rs#L1906-L1910🤖 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/index/prefilter.rs` around lines 234 - 239, Use a block-list-specific representation for overlay exclusions instead of accepting arbitrary RowAddrMask values. In rust/lance/src/index/prefilter.rs:234-239, update Prefilter's with_overlay_block to accept or validate RowAddrTreeMap, and at 398-400 combine only the validated block mask. In rust/lance/src/io/exec/filtered_read.rs:100-109, remove the release-mode silent no-op; at 1482-1487 store the block-list representation; at 1524-1529 validate the builder argument; and at 1906-1910 propagate validation errors when applying the exclusion.Source: Coding guidelines
… stores
commit_overlay wrote overlay files via Path::from("data/{name}"). For file://
stores, to_local_path() just prepends '/', making this resolve to /data/{name}
(root filesystem, Permission Denied). For memory:// stores it happened to work
because that branch doesn't call to_local_path at all.
Fix: use dataset.base.clone().join("data").join(filename) so that for file://
stores the path includes the dataset's base directory components, which when
prepended with '/' give the correct absolute path. For memory:// stores base
is the empty path, so join("data").join(name) produces the same string as before.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Previously, when any row in a fragment had a stale vector overlay, the entire fragment (up to 100k rows) was removed from ANN index coverage and re-scored on the flat path — adding ~1.9ms overhead per stale fragment in benchmarks. This changes the vector ANN path to row-level precision: - Stale row addresses are computed per-fragment (not whole-fragment) - A RowAddrMask::BlockList for stale rows is passed to DatasetPreFilter.overlay_block, blocking them from ANN results via the existing prefilter mechanism - Only the specific stale row addresses are re-scored via a targeted TakeExec + flat KNN path, not the whole fragment - Non-stale rows in the same fragment remain in ANN coverage For a fragment with 100k rows and 1 stale row, overhead drops from O(100k) flat KNN to O(1) targeted take. New infrastructure: - DatasetPreFilter::with_overlay_block for synchronous row-level blocks - ANNIvfSubIndexExec stores and applies overlay_block at execute time - new_knn_exec accepts Option<RowAddrMask> for overlay blocking - collect_overlay_stale_rows_for_segment: per-row stale computation - Scanner::mask_overlay_stale_rows: replaces mask_overlay_stale_segments Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
FTS queries could return stale hits when an overlay committed after the index build covers a fragment the FTS index also covers. The inverted index entries for those rows still reflect the pre-overlay values. Fix: in `plan_match_query`, compute which FTS segments touch stale fragments (overlay committed_version > segment.dataset_version on the indexed column) via `fts_stale_frags_and_fresh_segments`. Those segments are excluded from `MatchQueryExec` (using `new_with_segments` with the fresh subset) and all fragments they covered fall to the flat `FlatMatchQueryExec` path, which reads current overlay-merged values. The fast path (no overlays) is O(n fragments) with no segment loading. Adds two tests in overlay_index_masking: - test_fts_overlay_stale_drop_and_new_match: stale term no longer returned, new term found via flat path - test_fts_overlay_unrelated_field_not_excluded: overlay on a non-FTS field does not affect FTS coverage Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Previously, any stale overlay on a fragment caused the entire fragment to fall to the flat scan path (missing_frags), scanning up to 100k rows to re-evaluate the predicate for a single stale row. Now the BTree path matches the vector ANN approach: - partition_frags_by_coverage replaces the fragment-level overlay_stale_index_frags with overlay_stale_index_rows (row-level). Stale-but-indexed fragments stay in relevant_frags; only their per-row stale offsets are returned as a HashMap<u32, RoaringBitmap>. - MaterializeIndexExec gains overlay_block: Option<RowAddrMask> + with_overlay_block builder. The block mask is ANDed into the candidate mask before row_ids_for_mask, so stale index entries never reach downstream operators. - scalar_indexed_scan builds the block list from the stale row map and applies it to MaterializeIndexExec. A new stale-Take union path (OneShotExec(stale_row_ids) -> TakeExec -> LanceFilterExec(full_filter) -> project) re-evaluates only the stale rows against their current overlay-merged values and unions with the indexed path. Non-stale rows in a fragment with a stale overlay remain on the indexed path; only the specific stale rows pay the take cost. Adds test_btree_overlay_row_level_precision: overlays one row in a fragment so that two rows in the same fragment share the queried value. Verifies the non-stale row is returned (from the index) alongside the stale row (from the Take path). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…, cleanups Addresses review of the overlay index-masking work. - Scalar (V2 FilteredReadExec): previously the default v2 path computed per-row stale offsets, then collapsed them to fragment ids and dropped whole fragments to the flat path — only the legacy v1 scalar_indexed_scan path was row-level, and v1 cannot carry overlays. Block just the stale rows from the index result (via EvaluatedIndex::without_rows / FilteredReadOptions::overlay_block) and re-evaluate them with a targeted Take + full-filter path unioned with the indexed read, so v2 scalar masking is O(stale_rows), not O(fragment_size). - FTS phrase queries: plan_phrase_query loaded all segments unmasked, so a stale overlay could return stale phrase hits. Exclude segments covering stale fragments. Phrase has no flat re-evaluation path, so — like unindexed fragments, which phrase already does not search — overlaid fragments are dropped from the phrase result (matches surface after compaction); when every segment is excluded the query returns empty rather than erroring. Correct the stale fts() TODO that claimed FTS was unmasked. - Perf: stale-collection now iterates the (rare) overlaid fragments and probes segment coverage, O(overlaid_frags) instead of O(indexed_frags); the vector helper is scoped to the query's target fragments. - Cleanups: extract shared stale-rows block-mask and row-id-exec helpers (used by the scalar, vector, and v2 paths), encode row addresses via RowAddress::new_from_parts, relocate the collect_* helpers next to overlay_exclusion_offsets in dataset::overlay, rename mask_overlay_stale_rows -> overlay_stale_vector_rows, import HashMap, use RoaringBitmap over HashSet<u32>, and fix doc drift referencing overlay_stale_index_frags. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
scanner.rs had grown past 14.5k lines. Relocate the self-contained overlay_index_masking test module (~1000 lines) into the established dataset/tests/ directory as dataset_overlay_index_masking.rs. The tests use only crate-public APIs, so no visibility changes were needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Index results are in the row-id domain, and a physical row address equals its row id only when the dataset does not use stable row ids. The overlay stale-row masking expressed rows as physical addresses everywhere, so under stable row ids the block never matched the index results (stale hits leaked) and the re-scored rows were read wrong. Translate the stale addresses to the row-id domain (via translate_addr_treemap_to_row_ids) at every masking point: the scalar V2 take (FilteredReadExec), the scalar V1 block (MaterializeIndexExec), and the vector ANN prefilter (DatasetPreFilter). Parametrized scalar and vector tests over enable_stable_row_ids, overlaying a non-first fragment where address != row id. Also addresses PR review: - Use an address allow list (not a `_rowid` OneShotExec + TakeExec) to identify the stale re-eval rows; `_rowid` is a logical id under stable row ids, so the old path took the wrong rows. Consolidated into `stale_rows_take`. - Treat a missing `fragment_bitmap` as covering all fragments in the overlay collect helpers and in FTS segment classification, matching DatasetPreFilter, so stale rows can't slip through unmasked on legacy bitmap-less indices. - Gate the exact-index prefilter shortcut on there being no stale rows, so a stale selection vector can't reach ANN/FTS. - Replace ANNIvfSubIndexExec::try_new_with_overlay with a with_overlay_block builder. - Extract the shared version-gate + exclusion-offsets helper in overlay.rs. - Replace bare unwraps with `?`/expect_ok, debug_assert the block-list invariant in without_rows, and move test-only imports to the tests module. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add regression coverage for overlay index masking under fast_search and for the legacy missing-fragment_bitmap branch: - test_btree_overlay_masked_under_fast_search: the scalar drop-stale block and stale-Take re-eval both run regardless of fast_search. - test_vector_overlay_stale_dropped_under_fast_search: the ANN prefilter block drops the stale hit under fast_search while the flat re-score is skipped (recall tradeoff); guards against moving overlay_block inside the !fast_search guard. - test_collect_frags_missing_bitmap_covers_all / _rows: a None fragment_bitmap (index predating bitmap tracking) is treated as covering every fragment. Self-review cleanups: - Move the misplaced doc block onto collect_overlay_stale_frags (renamed from collect_stale_overlay_frags for consistency with the row-level collector); the private helper keeps its // comment. - Restore the "can't pushdown limit/offset" and "re-ordering anyways" comments dropped during the knn_combined rewrite. Extract create_vector_overlay_dataset/vector_query_ids helpers (shared with test_vector_index_rescore_on_overlay) and an ids_matching_opts fast_search variant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Overlay-aware index masking adds fields to widely-shared read-path types (e.g. DatasetPreFilter, ANNIvfSubIndexExec), which pushes the layout depth of several scan/take futures past rustc's default query-recursion limit (128) on 1.97. Raise the limit to 256 in the lance lib and in the one bench crate (mem_wal_point_lookup_bench) that exercises the deepened path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Self-review pass to reduce diff size without changing behavior: - Tests: merge the two FTS id-collecting helpers into one `fts_ids(query)` with thin term/phrase delegators, and extract `ids_from_batches` (the id-column -> Vec<i32> pattern was copied in 4 helpers). Hoist repeated in-fn `use` imports (StringArray, InvertedIndexParams, MetricType, VectorIndexParams, TryStreamExt) to the module top and drop a redundant IndexType re-import. - scanner: extract `overlaid_fragments` (the overlaid-fragment map was built 3x) and `stale_rows_in_id_domain` (shared tree-build + stable-row-id translation between the block-mask and take paths), consolidating the address-vs-rowid rationale into one place. - Trim the doc comments on the `with_overlay_block` setters to point at the field doc instead of repeating it, and shorten the `new_knn_exec` doc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…st nits Address review feedback: - Replace the `#![recursion_limit = "256"]` attributes (lance lib + the point- lookup bench) with `Box::pin` on the two deep futures that overlay-aware index masking tipped past rustc's default limit: the `remap_index` call in `remap_indices` and the bench's `plan_lookup`. Boxing caps the future's layout depth locally rather than papering over it crate-wide. - Drop `max_rows_per_group` from the test datasets — row groups don't exist past Lance 1.0. - Remove an unnecessary comment on `create_base_dataset_with`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…limit overflow
Overlay-aware index masking deepens create_plan's async layout. Two inline
callers push it past rustc's default recursion limit ("queries overflow the
depth limit!"), surfacing in coverage-instrumented CI builds:
- Scanner::explain_plan (fm_contains_bench)
- the mem_wal point-lookup scan builder (mem_wal_point_lookup_bench)
Box the future at each site, the same way try_into_stream and count_rows
already box their plan futures. Boxing per-site (rather than inside create_plan)
keeps create_plan's future type unchanged, avoiding an E0275 Send-bound
trait-solver overflow that centralized boxing triggered in downstream crates.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5c49fd7 to
6499de0
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/src/index/prefilter.rs (1)
46-61: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTypo: "ANDead" → "ANDed".
See consolidated comment.
🤖 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/index/prefilter.rs` around lines 46 - 61, Correct the typo in the overlay_block comment on DatasetPreFilter by changing “ANDead” to “ANDed”; leave the surrounding documentation and implementation unchanged.
🟡 Other comments (1)
rust/lance/src/io/exec/scalar_index.rs-663-666 (1)
663-666: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTypo: "ANDead" → "ANDed".
See consolidated comment.
🤖 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/scalar_index.rs` around lines 663 - 666, Correct the typo in the documentation for the overlay_block field by changing “ANDead” to “ANDed”; do not alter the field or surrounding explanation.
🧹 Nitpick comments (5)
rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs (5)
681-688: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
usestatement out of function body.
use lance_index::scalar::inverted::query::{FtsQuery, PhraseQuery};is declared insidefts_phrase_ids_matchingrather than at the top of the file.♻️ Proposed fix
+use lance_index::scalar::inverted::query::{FtsQuery, PhraseQuery}; + async fn fts_phrase_ids_matching(dataset: &Dataset, phrase: &str) -> Vec<i32> { - use lance_index::scalar::inverted::query::{FtsQuery, PhraseQuery}; - let query = FullTextSearchQuery::new_query(FtsQuery::Phrase(As per coding guidelines, "Place
useimports at the top of the file, not inline within function bodies."🤖 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 681 - 688, Move the lance_index query import for FtsQuery and PhraseQuery from inside fts_phrase_ids_matching to the file-level use declarations, leaving the function’s query construction and return behavior unchanged.Source: Coding guidelines
594-629: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame manual-construction pattern as
create_base_dataset_with.
create_text_datasetalso hand-buildsSchema/RecordBatch::try_newforInt32+Utf8columns, both supported byarrow_array::record_batch!(). Same guideline violation ascreate_base_dataset_with(lines 42-64) — see that comment for the proposed fix 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/tests/dataset_overlay_index_masking.rs` around lines 594 - 629, Update create_text_dataset to construct the Int32 and Utf8 columns using the arrow_array::record_batch!() helper instead of manually creating ArrowSchema, ArrowField, and RecordBatch::try_new. Preserve the existing row values, schema, and write behavior, including max_rows_per_file.Source: Coding guidelines
824-827: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame inline-
useissue in the benchmark function.
use std::time::Instant;anduse arrow_array::Float32Array;are declared insidebench_index_query_overlay_overheadrather than at the top of the file — same guideline violation asfts_phrase_ids_matching(lines 681-688).🤖 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 824 - 827, Move the inline imports std::time::Instant and arrow_array::Float32Array from bench_index_query_overlay_overhead to the file-level imports, matching the existing import organization used by fts_phrase_ids_matching. Remove the function-local use declarations while preserving the benchmark’s behavior.Source: Coding guidelines
42-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winManual
RecordBatchconstruction instead ofrecord_batch!().
create_base_dataset_withhand-buildsSchema/Arc/RecordBatch::try_newfor twoInt32columns. Confirmed via docs thatarrow_array::record_batch!macro supports Int32 columns and accepts variable/expression values, matching this exact case.♻️ Proposed refactor
async fn create_base_dataset_with(stable_row_ids: bool) -> Dataset { - let schema = Arc::new(ArrowSchema::new(vec![ - ArrowField::new("id", DataType::Int32, true), - ArrowField::new("age", DataType::Int32, true), - ])); - let batch = RecordBatch::try_new( - schema.clone(), - vec![ - Arc::new(Int32Array::from_iter_values(0..12)), - Arc::new(Int32Array::from_iter_values((0..12).map(|v| v * 10))), - ], - ) - .unwrap(); + let batch = record_batch!( + ("id", Int32, (0..12).collect::<Vec<_>>()), + ("age", Int32, (0..12).map(|v| v * 10).collect::<Vec<_>>()) + ) + .unwrap(); + let schema = batch.schema(); let write_params = WriteParams { max_rows_per_file: 6, enable_stable_row_ids: stable_row_ids, ..Default::default() }; - let reader = RecordBatchIterator::new(vec![Ok(batch)], schema.clone()); + let reader = RecordBatchIterator::new(vec![Ok(batch)], schema);The same pattern recurs in
create_text_dataset(Int32 + Utf8 columns), also a clean fit. As per coding guidelines, "Userecord_batch!()fromarrow_arrayto constructRecordBatchin tests instead of manualSchema/Arc/try_newboilerplate."🤖 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 42 - 64, Refactor create_base_dataset_with to construct the two-column RecordBatch using arrow_array::record_batch! instead of manually creating ArrowSchema, Arc-wrapped arrays, and RecordBatch::try_new. Apply the same record_batch! construction to create_text_dataset for its Int32 and Utf8 columns, preserving the existing values and schema.Source: Coding guidelines
553-590: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't actually exercise a compound predicate.
The docstring says "A compound boolean predicate (age AND id) exercises the ScalarIndexExpr tree-walk in
overlay_stale_index_rows", but the assertions only issue single-column filters ("age = 10","age = 999","id = 2") separately — never a combinedage = X AND id = Ypredicate. As written, this doesn't verify the claimed tree-walk behavior; a regression in compound-expression handling could slip through.🧪 Proposed addition
assert_eq!(ids_matching(&dataset, "age = 10").await, Vec::<i32>::new()); assert_eq!(ids_matching(&dataset, "age = 999").await, vec![1]); + // Compound predicate: exercises the AND tree-walk directly. + assert_eq!( + ids_matching(&dataset, "age = 999 AND id = 1").await, + vec![1] + ); // A pure `id` query on an unaffected fragment still works correctly. assert_eq!(ids_matching(&dataset, "id = 2").await, vec![2]);🤖 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 553 - 590, Update test_overlay_stale_with_compound_index_expression to issue assertions using combined age-and-id predicates, such as age = X AND id = Y, so ScalarIndexExpr tree-walking and stale-fragment fallback are exercised. Preserve coverage for the stale age=10 exclusion, the updated age=999 match, and the unaffected id behavior through compound filters rather than separate single-column queries.
🤖 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/index/prefilter.rs`:
- Around line 46-61: Correct the typo in the overlay_block comment on
DatasetPreFilter by changing “ANDead” to “ANDed”; leave the surrounding
documentation and implementation unchanged.
---
Other comments:
In `@rust/lance/src/io/exec/scalar_index.rs`:
- Around line 663-666: Correct the typo in the documentation for the
overlay_block field by changing “ANDead” to “ANDed”; do not alter the field or
surrounding explanation.
---
Nitpick comments:
In `@rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs`:
- Around line 681-688: Move the lance_index query import for FtsQuery and
PhraseQuery from inside fts_phrase_ids_matching to the file-level use
declarations, leaving the function’s query construction and return behavior
unchanged.
- Around line 594-629: Update create_text_dataset to construct the Int32 and
Utf8 columns using the arrow_array::record_batch!() helper instead of manually
creating ArrowSchema, ArrowField, and RecordBatch::try_new. Preserve the
existing row values, schema, and write behavior, including max_rows_per_file.
- Around line 824-827: Move the inline imports std::time::Instant and
arrow_array::Float32Array from bench_index_query_overlay_overhead to the
file-level imports, matching the existing import organization used by
fts_phrase_ids_matching. Remove the function-local use declarations while
preserving the benchmark’s behavior.
- Around line 42-64: Refactor create_base_dataset_with to construct the
two-column RecordBatch using arrow_array::record_batch! instead of manually
creating ArrowSchema, Arc-wrapped arrays, and RecordBatch::try_new. Apply the
same record_batch! construction to create_text_dataset for its Int32 and Utf8
columns, preserving the existing values and schema.
- Around line 553-590: Update test_overlay_stale_with_compound_index_expression
to issue assertions using combined age-and-id predicates, such as age = X AND id
= Y, so ScalarIndexExpr tree-walking and stale-fragment fallback are exercised.
Preserve coverage for the stale age=10 exclusion, the updated age=999 match, and
the unaffected id behavior through compound filters rather than separate
single-column queries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: a14fdaa6-9a0b-46d4-b421-c103b82836e2
📒 Files selected for processing (12)
rust/lance/benches/mem_wal/point_lookup/mem_wal_point_lookup_bench.rsrust/lance/src/dataset/index.rsrust/lance/src/dataset/mem_wal/scanner/point_lookup.rsrust/lance/src/dataset/overlay.rsrust/lance/src/dataset/scanner.rsrust/lance/src/dataset/tests/dataset_overlay_index_masking.rsrust/lance/src/dataset/tests/mod.rsrust/lance/src/index/prefilter.rsrust/lance/src/index/vector/fixture_test.rsrust/lance/src/io/exec/filtered_read.rsrust/lance/src/io/exec/knn.rsrust/lance/src/io/exec/scalar_index.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
Functionally, I approve this. I think it's doing the right thing.
I suspect there is some room for improvement at some point. It feels disjoint that the deleted rows mask and the fragment white-list mask are computed by the nodes themselves (I think?) but the overlay mask is computed by the scanner.
We have this one concept (index results have to mask out invalidated entries) but it is getting pieced together all over the place. It feels like there is some opportunity to centralize all this logic someday.
However, this is probably the perfect getting in the way of the good, so let's reserve for a potential follow-up.
| // Box this deep future: overlay-aware index masking pushes its layout past rustc's | ||
| // default recursion limit when inlined here. | ||
| let plan = Box::pin(planner.plan_lookup(&[ScalarValue::Int64(Some(id))], None)).await?; |
There was a problem hiding this comment.
Fine, I guess, but the comment probably doesn't need to mention this specific feature as the thing that pushed it over the edge.
Still, it feels like plan_lookup should probably be boxed, and not this particular call site.
There was a problem hiding this comment.
Removed this call-site box — plan_lookup stays shallow now because the box moved into create_plan (8d45b3a). Also dropped the feature-specific wording per your note.
There was a problem hiding this comment.
Small correction: the box is gone here (c8835ff), but not by moving it into create_plan — that hits E0275 (see the create_plan thread). It's simply redundant now: build_source_scan already boxes its create_plan calls, so plan_lookup stays shallow on its own.
| // Box the plan-building future: overlay-aware index masking deepens | ||
| // create_plan's async layout past rustc's default recursion limit | ||
| // when this scan builder is awaited inline up the point-lookup chain. | ||
| Box::pin(scanner.create_plan()).await? |
There was a problem hiding this comment.
Same as above, we should probably be boxing create_plan and not all the call sites.
Also, it seems an odd coincidence that overlay aware index masking burst two unrelated methods. Is it possible there is something in here that is unnecessarily heavy on the stack space?
There was a problem hiding this comment.
Done in 8d45b3a — boxed the source-builder arms inside create_plan (vector/fts/take/filtered_read) rather than at the call sites. That shrinks create_plan's own future, so every inline caller (explain_plan, this scan builder, plan_lookup, try_into_stream, count_rows) stops overflowing with zero call-site boxes. On the "heavy on the stack" hunch — good call; running -Zprint-type-sizes to see whether one node holds a large variant across an await we could trim instead, will report back here.
There was a problem hiding this comment.
Correction to my note above — I tried boxing inside create_plan and it does not work: it reintroduces exactly the E0275 you were suspicious about (clippy/linux-arm/build-no-lock/linux-build all went red with portable_atomic::…NotRefUnwindSafe: Send overflow, demanded from lance-namespace-impls).
That's the bottom of it: an inline .await proves Send by structural derivation, but Box::pin(fut) turns it into a Box<Future>: Send trait obligation that the solver proves by recursing through the future's captured moka cache types — which overflows from a downstream crate with a long obligation chain. So the layout-depth overflow and the E0275 are the same deep async chain seen two ways, and the box has to stay at the call site (it only changes layout, not the Send derivation form). Reverted to call-site boxing in c8835ff (clippy now green). Notably the E0275 only reproduces under --features ALL_FEATURES, which is why it slipped past a default-feature check.
On "heavy on the stack" — -Zprint-type-sizes says no: create_plan's future is ~3.8 KB and the biggest futures (~13 KB) are the index-build/remap chains. It's call-chain depth, not one oversized node, so there's nothing to trim to avoid the boxing.
| scanner.filter_expr(filter.clone()); | ||
| scanner.create_plan().await? | ||
| // Box the plan-building future: overlay-aware index masking deepens | ||
| // create_plan's async layout past rustc's default recursion limit |
There was a problem hiding this comment.
Recursion limit? Or future size? How does boxing help with recursion limits?
There was a problem hiding this comment.
It's the layout-computation query depth — not #![recursion_limit] (trait/type recursion) or E0275 (trait-solver depth). An async fn embeds each awaited sub-future's state machine inline to compute its own size, so a chain of N inline awaits makes the layout query recurse N deep; the overlay fields tipped an already-near-128 read-path chain over. Box::pin puts the sub-future behind a pointer, so the parent's layout stops recursing into it (the box is sized as an independent query). It reproduces on a plain cargo check on 1.94.0 and clears with the boxing.
| let remap_result = self.remap_index(index, &mapping).await?; | ||
| // Box this deep future: overlay-aware index masking pushes its layout past | ||
| // rustc's default recursion limit when inlined into this state machine. | ||
| let remap_result = Box::pin(self.remap_index(index, &mapping)).await?; |
There was a problem hiding this comment.
Ok, now I'm really suspicious. We should get to the bottom of this. Also, if we are certain this is inevitable, as above, let's box remap_index and not all the call sites.
There was a problem hiding this comment.
Yeah I will look into pushing the box up. Good nudge.
There was a problem hiding this comment.
Boxed inside the remap_index method itself now (8d45b3a), so remap_indices needs no call-site box. Root cause is the async-layout query depth (see the point_lookup thread) — not really a coincidence: both create_plan and the remap chain run through the read/take machinery the overlay fields deepened.
There was a problem hiding this comment.
Correction: boxing inside remap_index hits the same E0275 as create_plan (see that thread), so this stays a call-site box (c8835ff). And it isn't coincidental that both burst — both chains run through the same cache/IO machinery whose captured types overflow the Box<Future>: Send proof once the future is boxed.
| // Row addresses whose index entries are stale due to a newer data overlay committed after | ||
| // the index was built. Computed synchronously at plan time and ANDead into the final mask | ||
| // so the index never returns those rows. | ||
| pub(super) overlay_block: Option<RowAddrMask>, |
There was a problem hiding this comment.
(not a problem) I think DatasetPreFilter is a misleading name at this point 😆
There was a problem hiding this comment.
Agreed it's misleading now that it assembles the full plan-time mask. Rather than churn a wide rename into this PR, I've folded it into the centralization follow-up #7898.
| fn without_rows(mut self, block: &RowAddrMask) -> Self { | ||
| // `overlay_block` is always constructed as a block list (see `Scanner::stale_rows_block_mask`). | ||
| let block_list = block.block_list(); |
There was a problem hiding this comment.
Should this method just take a RowAddrTreeMap so we force the assertion on the caller?
There was a problem hiding this comment.
Done — without_rows now takes &RowAddrTreeMap and the caller extracts block_list(), so the block-list invariant lives at the caller and the internal debug_assert is gone (8d45b3a).
| /// have no live counterpart and are dropped, which is correct: those rows are | ||
| /// not part of the answer. | ||
| async fn translate_addr_treemap_to_row_ids( | ||
| pub(crate) async fn translate_addr_treemap_to_row_ids( |
There was a problem hiding this comment.
Why is this becoming pub(crate)? If we need it elsewhere let's move it into a utility file.
There was a problem hiding this comment.
Moved translate_addr_treemap_to_row_ids to dataset::rowids (next to load_row_id_sequence) instead of widening its visibility here (8d45b3a).
Hmm I thought the fragment white-list mask was computed at the scanner level at least. Deleted rows and invalidated rows are different. Deleted rows should be ignored both by index search and brute force search. Invalidated rows need to be moved from index search into brute force search. So it makes sense they would have different mechanisms to me. That being said, scanner.rs is definitely getting out of hand. I grimace whenever I touch it. GitHub won't even do syntax highlighting on the diff anymore (at least for me) because it's so big. |
There was a problem hiding this comment.
Actionable comments posted: 1
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/scanner.rs (1)
3585-3617: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRe-evaluate stale phrase fragments instead of dropping them.
flat_frag_idsis discarded, so an overlay that introduces a phrase match is never returned until compaction. Build a flat phrase fallback for those fragments and union it with the fresh-segment indexed result, as match queries do.🤖 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 3585 - 3617, Update the phrase-query execution flow around fts_stale_frags_and_fresh_segments to retain flat_frag_ids and re-evaluate those stale fragments through the flat phrase path instead of dropping them. Build the flat phrase fallback for stale fragments and union its results with the fresh-segment PhraseQueryExec output, matching the existing match-query behavior while preserving the empty result when neither path has searchable fragments.
🤖 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/rowids.rs`:
- Around line 138-145: Update the row-id iteration around ids.next() so a None
result for a selected physical_offset returns Error::corrupt_file instead of
breaking. Include fragment_id, physical_offset, and the relevant row counts in
the corruption error, while preserving insertion for available IDs and normal
completion when no selected offset is truncated.
---
Outside diff comments:
In `@rust/lance/src/dataset/scanner.rs`:
- Around line 3585-3617: Update the phrase-query execution flow around
fts_stale_frags_and_fresh_segments to retain flat_frag_ids and re-evaluate those
stale fragments through the flat phrase path instead of dropping them. Build the
flat phrase fallback for stale fragments and union its results with the
fresh-segment PhraseQueryExec output, matching the existing match-query behavior
while preserving the empty result when neither path has searchable fragments.
🪄 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: 57c8b89c-c760-4753-b692-76ac91d63d4a
📒 Files selected for processing (5)
rust/lance/src/dataset/index.rsrust/lance/src/dataset/rowids.rsrust/lance/src/dataset/scanner.rsrust/lance/src/io/exec/filtered_read.rsrust/lance/src/io/exec/scalar_index.rs
| match ids.next() { | ||
| Some(id) => { | ||
| if offsets.contains(physical_offset) { | ||
| row_ids.insert(id); | ||
| } | ||
| } | ||
| None => break, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return corruption error when the row-id sequence is truncated.
None => break silently omits selected stale offsets when metadata is inconsistent, letting stale index results escape masking. Return Error::corrupt_file with fragment_id, physical_offset, and row counts instead.
🤖 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/rowids.rs` around lines 138 - 145, Update the row-id
iteration around ids.next() so a None result for a selected physical_offset
returns Error::corrupt_file instead of breaking. Include fragment_id,
physical_offset, and the relevant row counts in the corruption error, while
preserving insertion for available IDs and normal completion when no selected
offset is truncated.
Source: Coding guidelines
Keep the async-layout recursion boxing at the call sites (explain_plan, the point-lookup scan builder, remap_indices) rather than inside create_plan / remap_index: boxing *inside* those methods turns the future's `Send` check into a `Box<Future>: Send` trait obligation that overflows the solver through the moka cache types (E0275 in downstream crates). Dropped the now-redundant `plan_lookup` box in the bench — the scan-builder boxes already keep that chain shallow. Reworded the boxing comments to explain the mechanism. Review nits: - `EvaluatedIndex::without_rows` takes `&RowAddrTreeMap` (the block-list invariant moves to the caller) instead of asserting internally. - Move `translate_addr_treemap_to_row_ids` to `dataset::rowids`, next to `load_row_id_sequence`, instead of widening its visibility in scalar_index. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8d45b3a to
c8835ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/lance/src/io/exec/filtered_read.rs (1)
1564-1567: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the new public overlay-mask builders. Both builders need API docs describing block-list semantics, effects on index candidates, and a synchronized example linking to the relevant mask type.
rust/lance/src/io/exec/filtered_read.rs#L1564-L1567: add docs and an example forFilteredReadOptions::with_overlay_block.rust/lance/src/io/exec/scalar_index.rs#L684-L687: add docs and an example forMaterializeIndexExec::with_overlay_block.🤖 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/filtered_read.rs` around lines 1564 - 1567, Add API documentation to FilteredReadOptions::with_overlay_block in rust/lance/src/io/exec/filtered_read.rs:1564-1567 and MaterializeIndexExec::with_overlay_block in rust/lance/src/io/exec/scalar_index.rs:684-687, describing block-list semantics, effects on index candidates, and including synchronized examples that reference the relevant mask type in each module.Source: Coding guidelines
rust/lance/src/dataset/scanner.rs (2)
3578-3610: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not drop stale phrase-query fragments without re-evaluating them.
This path excludes every stale segment and returns only fresh indexed hits, so a phrase newly introduced or modified by an overlay is omitted even when
fast_searchis false. Match queries correctly route stale fragments through flat evaluation; phrase queries need equivalent fallback semantics (and an overlay phrase regression test) before masking is enabled.🤖 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 3578 - 3610, Update the phrase-query execution flow around fts_stale_frags_and_fresh_segments and PhraseQueryExec so stale fragments are re-evaluated through the flat path instead of being excluded, including when fast_search is false. Preserve indexed execution for fresh segments, and add a regression test covering phrase matches introduced or modified by an overlay before enabling any stale-fragment masking.Source: Coding guidelines
4487-4492: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winFail instead of silently skipping stale-row re-evaluation.
unwrap_or_default()converts an unexpected absentrow_addrs()iterator into an empty take input. The rows remain blocked from the index but are never re-evaluated. Return an internal error containing the stale fragment and row counts instead.🤖 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 4487 - 4492, Update the stale-row re-evaluation flow around stale_rows_in_id_domain and take_id_map.row_addrs() so an absent row-address iterator returns an internal error instead of defaulting to an empty take input. Include the stale fragment and relevant row counts in the error, while preserving normal take-input construction when row_addrs() is present.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/rowids.rs`:
- Around line 119-146: Update the row-ID selection logic around the physical
row-count conversion and loop in the enclosing method: convert the result of
file_fragment.physical_rows() with u32::try_from, propagating conversion
failure, then reject max_offset values greater than or equal to
num_physical_rows by returning Error::corrupt_file containing fragment_id,
max_offset, and num_physical_rows. Preserve normal iteration for valid offsets.
---
Outside diff comments:
In `@rust/lance/src/dataset/scanner.rs`:
- Around line 3578-3610: Update the phrase-query execution flow around
fts_stale_frags_and_fresh_segments and PhraseQueryExec so stale fragments are
re-evaluated through the flat path instead of being excluded, including when
fast_search is false. Preserve indexed execution for fresh segments, and add a
regression test covering phrase matches introduced or modified by an overlay
before enabling any stale-fragment masking.
- Around line 4487-4492: Update the stale-row re-evaluation flow around
stale_rows_in_id_domain and take_id_map.row_addrs() so an absent row-address
iterator returns an internal error instead of defaulting to an empty take input.
Include the stale fragment and relevant row counts in the error, while
preserving normal take-input construction when row_addrs() is present.
In `@rust/lance/src/io/exec/filtered_read.rs`:
- Around line 1564-1567: Add API documentation to
FilteredReadOptions::with_overlay_block in
rust/lance/src/io/exec/filtered_read.rs:1564-1567 and
MaterializeIndexExec::with_overlay_block in
rust/lance/src/io/exec/scalar_index.rs:684-687, describing block-list semantics,
effects on index candidates, and including synchronized examples that reference
the relevant mask type in each module.
🪄 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: 36af5aae-2e57-48c7-a516-1e77806580b0
📒 Files selected for processing (6)
rust/lance/src/dataset/index.rsrust/lance/src/dataset/mem_wal/scanner/point_lookup.rsrust/lance/src/dataset/rowids.rsrust/lance/src/dataset/scanner.rsrust/lance/src/io/exec/filtered_read.rsrust/lance/src/io/exec/scalar_index.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
The CodeRabbit-suggested validation in translate_addr_treemap_to_row_ids called Error::corrupt_file with a single message argument, but that constructor takes (path, message) — breaking every build. Switch to Error::internal, matching the sibling error already in this function. Also address the companion CodeRabbit comment: the `None` arm of `ids.next()` used to `break`, silently dropping selected stale offsets when the row-id sequence holds fewer ids than the fragment has live rows. That lets stale index results escape masking, so return an error on the mismatch instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Index masking for data overlay files. Builds on the overlay read path (#7536), now merged.
Problem
An index built before an overlay does not reflect the overlay's values, so its entries for overlay-covered cells may be stale.
WHERE age = 25after an overlay sets a row's age to 26 must not return that row from the index;WHERE age = 26must find it. Queries must stay correct while overlays remain.Approach
For each index a query relies on, compute which row addresses within each covered fragment carry an overlay committed after the index (
committed_version > index.dataset_version) and touching a field the index covers. The check is:committed_version <= index.dataset_version) is ignored.This is the new
overlay_exclusion_offsetshelper indataset/overlay.rs.Such rows are excluded from the index result and re-evaluated against their current (overlay-merged) values:
MaterializeIndexExecoutput viaoverlay_block: Option<RowAddrMask>. Only those rows are re-evaluated via a targetedTakeExec+ full-filter path. Non-stale rows in the same fragment remain on the indexed path.DatasetPreFilter.overlay_block, blocking them from ANN results. Only those rows are re-scored via a targetedTakeExec+ flat-KNN path. Non-stale rows in the same fragment stay in ANN coverage.MatchQueryExec(vianew_with_segments). Stale fragments fall to the existingFlatMatchQueryExecpath, which reads current overlay-merged values. Fragment granularity is sufficient because flat text matching is row-precise and cheap relative to flat-KNN.This drops stale index hits and surfaces new matches the index never saw.
Performance
Benchmark: 1M rows, 100k rows/fragment (10 frags), 32-dim vec, NVMe, release build.
Scalar (BTree) —
age = <value>The overhead when overlays exist comes from the stale-check itself (segment metadata load), not from flat-scanning — only stale rows are taken, not the whole fragment.
Vector ANN — fragment 0 with 1 overlaid row
Previously at fragment-level: 10.0ms → 11.9ms (+1.9ms scanning 100k rows flat).
With row-level: +0.8ms for targeted take of 1 row — overhead is O(stale_rows), not O(fragment_size).
Fast-path summary:
overlay_exclusion_offsetscalled once per covered fragment with overlays.Tests
dataset::tests::dataset_overlay_index_masking(e2e) anddataset::overlay(unit):committed_version <= index.dataset_versionnot excludedoverlay_exclusion_offsetsunit tests: version gate, field-awareness, sparse per-field coverage, multi-overlay union🤖 Generated with Claude Code