fix(index): correct offset→row-id translation for overlay masking under deletions by wjones127 · Pull Request #7918 · lance-format/lance

wjones127 · 2026-07-22T16:14:23Z

Stacked on #7549.

Under stable row ids, a scalar (BTREE) index query could return wrong results when a fragment had both a deletion and an overlay on the indexed column, if the deleted row's physical offset was below the overlaid row's offset in the same fragment.

translate_addr_treemap_to_row_ids maps a stale overlay row's physical offset to its stable row id when building the overlay block/take set. It advanced a RowIdSequence::iter() cursor only for non-deleted offsets, assuming the sequence yields ids for live rows only. It does not: the sequence keeps one id per physical row, and deletions are tracked separately by the deletion vector without compacting the sequence (RowIdIndex maps ids to start_address + position and filters deletions separately). A deletion at an offset below a stale offset therefore desynced the cursor, mapping the stale offset to the wrong row id — so the overlay block masked the wrong row, the stale index hit leaked, and the row's new overlay value was never surfaced.

Fix: index the sequence directly by physical offset (as row_addrs_to_row_ids already does), skipping deleted offsets. Also cheaper — O(stale offsets) instead of O(physical rows).

Only affects datasets with stable row ids; without them, addresses are row ids and no translation happens.

Regression test test_btree_overlay_stale_row_with_prior_deletion (parametrized over stable_row_ids) overlays fragment 1 and deletes a row below the stale offset. Fails before the fix on the stable-row-id case (age = 80 leaks the stale hit), passes after; the non-stable case is a control that stays green.

🤖 Generated with Claude Code

A scalar or vector index built before an overlay does not reflect the overlay's values, so its entries for overlay-covered cells may be stale. Queries must stay correct while overlays remain. For each index a query relies on, compute the set of fragments carrying an overlay that was committed after the index (`committed_version > index.dataset_version`) and that touches a field the index covers. The check is field-aware (an overlay on an unrelated field excludes nothing) and version-gated (an overlay already incorporated by the index is ignored), via the new `overlay_exclusion_offsets` helper. Such fragments are dropped from the index's covered set so they fall to the flat path, which re-evaluates them against their current (overlay-merged) values: - Scalar (`FilteredReadExec`): stale fragments are removed from the `EvaluatedIndex` covered set, so the full filter is re-applied to them. - Vector: stale fragments are removed from the index segments' coverage bitmaps (so the ANN prefilter blocks their stale rows) and added to the flat-KNN fallback so their current vectors are re-scored into the top-k. This drops stale index hits and surfaces new matches the index never saw. Granularity is per fragment; row-level exclusion is a future optimization. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Two optimizations to reduce per-query overhead of the overlay-stale-index check: 1. `mask_overlay_stale_segments` now returns `Option<Vec<IndexMetadata>>` instead of always cloning the segment list. `None` means "use the original slice unchanged" — no allocation on the fast path (no overlays anywhere or no stale fragments found). Only the path that actually filters stale rows from coverage bitmaps clones the segments. 2. `collect_stale_overlay_frags` adds a cheap version pre-check before calling `overlay_exclusion_offsets`: if every overlay on a fragment predates the index (`committed_version <= segment.dataset_version`), the fragment is skipped without loading any coverage bitmaps. Also adds an `#[ignore]` benchmark (`bench_index_query_overlay_overhead`) covering BTree, FTS, and vector ANN queries at 0/4/16 overlay layers so the overhead can be measured directly. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

Address understandability gaps flagged in self-review: - `overlay_stale_index_frags`: explain that `ScalarIndexExpr` has no built-in visitor (motivating the manual stack traversal), and note that `load_named_scalar_segments` hits the cached index manifest so the per-leaf cost is bounded. - `partition_frags_by_coverage`: explain why stale fragments do not need to be threaded downstream as in the vector path — the scalar flat path already handles them via `missing_frags`, making explicit re-scoring unnecessary. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

Two additions from polish-pr self-review: 1. `test_overlay_stale_with_compound_index_expression`: exercises the ScalarIndexExpr tree-walk in `overlay_stale_index_frags` with an AND predicate over two independently-indexed columns (age AND id). Closes the coverage gap where no e2e test covered multi-leaf index expressions. 2. `TODO` comment in the `fts` method noting that FTS does not yet mask overlay files — a stale FTS index entry for an overlaid text field would survive until compaction. This is out of scope here but documents the known gap and records what the fix would require. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

Replace the 1,500-row in-memory benchmark with a realistic 1M-row disk-based harness (10 fragments × 100k rows, 32-dim vectors). Two scenarios that exercise the actual correctness overhead: Scenario A (BTree): overlay on `age` (the indexed field). Fragment 0 falls to a 100k-row flat scan instead of an O(log n) index lookup. Measures `cold_frag0_ms` (stale fragment) vs `warm_frag1_ms` (index-served) at 0/1/4/16 overlay layers to isolate flat-scan cost from index-lookup baseline. Scenario B (Vector ANN): overlay on `vec` (the indexed field). The field-aware check confirms the BTree age overlays leave the vector index clean. One vec overlay on fragment 0 forces brute-force KNN across all 100k rows of that fragment (O(100k × 32) extra distance computations). Measures `ann_ms` at 0 vs 1 vec overlay. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>

… 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>

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>

…er deletions `translate_addr_treemap_to_row_ids` mapped a stale overlay row's physical offset to its stable row id by advancing a `RowIdSequence::iter()` cursor only for non-deleted offsets, assuming the sequence yields ids for live rows only. It does not: the sequence keeps one id per physical row, and deletions are tracked separately by the deletion vector without compacting the sequence (see `RowIdIndex`, which maps ids to `start_address + position` and filters deletions separately). A deletion at an offset below a stale offset in the same fragment therefore desynced the cursor, mapping the stale offset to the wrong row id: the overlay block masked the wrong row, so the stale BTREE index hit leaked and the row's new overlay value was never surfaced. Index the sequence directly by physical offset (as `row_addrs_to_row_ids` already does), skipping deleted offsets. This is also cheaper — O(stale offsets) instead of O(physical rows). Only affects datasets with stable row ids; without stable row ids addresses are row ids and no translation happens. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-07-22T16:14:33Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: 37f02cab-7a4b-4d58-95b2-46c05ca61f12

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch will/oss-1325-fix-deletion-rowid-translate

_{Comment @coderabbitai help to get the list of available commands.}

wjones127 and others added 19 commits July 20, 2026 16:46

github-actions Bot added the bug Something isn't working label Jul 22, 2026

Base automatically changed from will/oss-1325-indexes-mask-data-overlay-files-correctly to main July 22, 2026 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(index): correct offset→row-id translation for overlay masking under deletions#7918

fix(index): correct offset→row-id translation for overlay masking under deletions#7918
wjones127 wants to merge 19 commits into
mainfrom
will/oss-1325-fix-deletion-rowid-translate

wjones127 commented Jul 22, 2026

Uh oh!

coderabbitai Bot commented Jul 22, 2026

Review skipped

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

wjones127 commented Jul 22, 2026

Uh oh!

coderabbitai Bot commented Jul 22, 2026

Review skipped

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant