fix(index): keep data overlays masked through OptimizeIndices

[wjones127]

Problem

Overlay index masking (#7549) gates on overlay.committed_version > segment.dataset_version. The merge path of OptimizeIndices carries an old segment's stale pre-overlay entries without re-reading overlaid values, yet stamped the merged segment with the current dataset_version. That flips the gate off and un-masks the overlay: the stale index entry resurfaces and the overlaid value is dropped.

Confirmed for every index type — BTREE, BITMAP, FTS, and IVF vector. For example, after overlaying age 10 → 999 on an indexed row and then merging:

• age = 10 returns the row again (stale entry resurfaced)
• age = 999 returns nothing (overlaid value dropped)

The delta/append path was already safe (keeps the old segment with its old version), as is rebuild/retrain-from-scan (re-reads and incorporates the overlay). Only merge-without-re-read was affected.

Fix

Treat an overlay-stale fragment — one touched by a field-relevant overlay committed after the segment was built — like a compaction-retired fragment during merge, mirroring the existing prune_overlay_stale_fields_from_indices for compaction.

• New segment_coverage_split drops overlay-stale fragments from the merged segment's coverage.
• Scalar types (BTree / bitmap / label_list / FTS) additionally filter those fragments' old entries out via OldIndexDataFilter.
• Vector needs only the coverage exclusion, because the ANN prefilter is already an allow-list restricted to coverage (DatasetPreFilter::new → create_restricted_deletion_mask), whereas the scalar MaterializeIndexExec prefilter is not.

Pruned fragments fall to the flat path and read current (overlay-merged) values, so both the stale-drop and the new-match come out correct.

Tests

test_optimize_preserves_* in dataset_overlay_index_masking.rs reproduce the un-masking and assert it stays masked across BTREE, BITMAP, FTS, and IVF vector.

🤖 Generated with Claude Code

[coderabbitai]

📝 Walkthrough

Walkthrough

Index append and merge logic now treats overlay-stale fragments as retired for scalar, FTS, and vector indexes. New regression tests verify stale entries remain masked after index optimization.

Changes

Overlay index masking

Layer / File(s)	Summary
Overlay-aware fragment coverage rust/lance/src/index/append.rs	Coverage splitting now returns effective and retired fragments, including fragments with overlays not incorporated by the merge.
Retired coverage in index merges rust/lance/src/index/append.rs	Scalar, inverted, and vector merge paths remove overlay-stale postings and vector coverage using retired fragments.
Optimization masking regression tests rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs	Tests verify scalar, FTS, and vector overlay masking before and after optimize_indices.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Dataset
  participant OverlayState
  participant IndexMerge
  participant Query
  Dataset->>OverlayState: identify overlay-stale fragments
  Dataset->>IndexMerge: optimize index segments
  IndexMerge->>OverlayState: classify stale fragments as retired
  IndexMerge->>Query: publish merged index without stale coverage
  Query->>Dataset: resolve overlaid values through flat path

Loading

Possibly related PRs

• lance-format/lance#7549: Extends related overlay-stale regression coverage and fragment masking logic.
• lance-format/lance#7772: Also changes index coverage handling for overlay materialization and stale fragment removal.

Suggested labels: A-index

Suggested reviewers: westonpace, xuanwo

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: preserving overlay masking during OptimizeIndices index merges.
Description check	✅ Passed	The description accurately explains the bug, fix, and tests for the overlay-masking merge issue.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[wjones127]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (3)

rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs (2)

1095-1098: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move inline use statements to the top of the file.

use crate::index::DatasetIndexExt;, use lance_index::optimize::OptimizeOptions;, and use lance_index::scalar::BuiltinIndexType; are declared inside the test function body. The same pattern repeats in test_optimize_preserves_fts_overlay_masking (lines 1152-1153) and test_optimize_preserves_vector_overlay_masking (lines 1220-1221). Hoist these to the file-level imports.

As per coding guidelines, "Place use imports at the top of the file, not inline within function bodies."

♻️ Proposed fix

-async fn test_optimize_preserves_scalar_overlay_masking(#[case] index_type: IndexType) {
-    use crate::index::DatasetIndexExt;
-    use lance_index::optimize::OptimizeOptions;
-    use lance_index::scalar::BuiltinIndexType;
-
+async fn test_optimize_preserves_scalar_overlay_masking(#[case] index_type: IndexType) {

(and add the three use statements near the top of the file, alongside the corresponding removals in the other two tests)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs` around lines
1095 - 1098, Hoist DatasetIndexExt, OptimizeOptions, and BuiltinIndexType into
the file-level imports, then remove their inline use statements from
test_optimize_preserves_scalar_overlay_masking,
test_optimize_preserves_fts_overlay_masking, and
test_optimize_preserves_vector_overlay_masking.

Source: Coding guidelines

---

1216-1286: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Missing regression coverage for the stable-row-id path.

All three new regression tests exercise only uses_stable_row_ids() == false (this test explicitly passes create_vector_overlay_dataset(false); the scalar/FTS tests don't set enable_stable_row_ids either). In append.rs, build_old_data_filter takes a different code path when stable row IDs are enabled (build_stable_row_id_filter, which derives its allow-list purely from effective_old_frags) — this branch is exactly where the overlay-stale exclusion from segment_coverage_split needs to propagate correctly, and it's untested by this PR.

Since create_vector_overlay_dataset already accepts a stable_row_ids flag, consider parameterizing at least one of these tests (or adding a stable-row-id variant) to cover that branch too.

🤖 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
1216 - 1286, Extend the regression coverage around
test_optimize_preserves_vector_overlay_masking to also run with
create_vector_overlay_dataset(true), exercising the stable-row-ID branch in
build_old_data_filter and build_stable_row_id_filter. Preserve the existing
assertions that the stale id=35 remains excluded and overlaid id=40 remains
discoverable after optimization, while keeping the non-stable case covered.

rust/lance/src/index/append.rs (1)

439-442: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Redundant coverage-split computation for BTree merges.

split_segment_coverage (line 441-442) already calls segment_coverage_split (and thus overlaid_fragments(&dataset.manifest.fragments)) once per segment in selected_old_indices to build the aggregate. For the IndexType::BTree branch, build_per_segment_filters (line 516-517) then re-runs overlaid_fragments and segment_coverage_split for the same segments to get the per-segment filters — retired_old_frags from line 441 is never even used on this branch. The Inverted merge path below (lines 900-912) shows the more efficient pattern: compute overlaid_fragments once and loop segment_coverage_split manually to get both per-segment and aggregated results in one pass.

Consider threading the already-computed overlaid map (or the per-segment splits) through to build_per_segment_filters to avoid the duplicate O(fragments) scan and duplicate per-segment bitmap work on every BTree merge.

Also applies to: 514-527

🤖 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/append.rs` around lines 439 - 442, Avoid recomputing
segment coverage for BTree merges: update the flow around split_segment_coverage
and build_per_segment_filters to reuse the already computed overlaid fragment
map or per-segment coverage splits for selected_old_indices. Preserve the
existing aggregate coverage behavior and per-segment filter results while
eliminating the second overlaid_fragments and segment_coverage_split pass; keep
the Inverted merge path’s single-pass pattern as the model.

🤖 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.

Nitpick comments:
In `@rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs`:
- Around line 1095-1098: Hoist DatasetIndexExt, OptimizeOptions, and
BuiltinIndexType into the file-level imports, then remove their inline use
statements from test_optimize_preserves_scalar_overlay_masking,
test_optimize_preserves_fts_overlay_masking, and
test_optimize_preserves_vector_overlay_masking.
- Around line 1216-1286: Extend the regression coverage around
test_optimize_preserves_vector_overlay_masking to also run with
create_vector_overlay_dataset(true), exercising the stable-row-ID branch in
build_old_data_filter and build_stable_row_id_filter. Preserve the existing
assertions that the stale id=35 remains excluded and overlaid id=40 remains
discoverable after optimization, while keeping the non-stable case covered.

In `@rust/lance/src/index/append.rs`:
- Around line 439-442: Avoid recomputing segment coverage for BTree merges:
update the flow around split_segment_coverage and build_per_segment_filters to
reuse the already computed overlaid fragment map or per-segment coverage splits
for selected_old_indices. Preserve the existing aggregate coverage behavior and
per-segment filter results while eliminating the second overlaid_fragments and
segment_coverage_split pass; keep the Inverted merge path’s single-pass pattern
as the model.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: 6ef5ce11-c3be-49be-b16a-f50d343e336e

📥 Commits

Reviewing files that changed from the base of the PR and between 4104324 and a4317f9.

📒 Files selected for processing (2)

• rust/lance/src/dataset/tests/dataset_overlay_index_masking.rs
• rust/lance/src/index/append.rs

[codecov]

Codecov Report

❌ Patch coverage is 87.50000% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
rust/lance/src/index/append.rs	87.50%	0 Missing and 6 partials ⚠️

📢 Thoughts on this report? Let us know!