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#7918wjones127 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesUpdated stable row ID translation to process selected physical offsets directly, skip deleted offsets, and report missing sequence entries as corruption. Added an end-to-end BTree overlay masking regression test covering stale, updated, retained, and deleted rows. Stable row ID overlay masking
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…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>
c9d2bb2 to
a064bad
Compare
|
@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.
🧹 Nitpick comments (1)
rust/lance/src/dataset/rowids.rs (1)
119-142: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCorrectly fixes the offset→row-id desync; missing test for the new corruption guard.
The direct
sequence.get(physical_offset)lookup correctly replaces the old cursor-based iteration, matching the PR's regression test indataset_overlay_index_masking.rs. However, the newok_or_elsebranch (Line 135-141) that errors on a missing sequence entry is itself new behavior (previously such offsets would presumably fall through silently) and has no test exercising it — e.g., a scenario where a stale offset from an index result no longer maps to any entry in the currentRowIdSequence.As per coding guidelines, "Every bugfix and feature must have corresponding 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/rowids.rs` around lines 119 - 142, The corruption guard in the offset-to-row-id mapping needs a regression test for offsets beyond the current RowIdSequence. Extend the relevant row-id masking test coverage around the sequence lookup in the fragment processing flow to construct a stale physical offset with no corresponding sequence entry, then assert the operation returns the expected internal error and preserves the reported fragment, offset, and sequence-length details.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.
Nitpick comments:
In `@rust/lance/src/dataset/rowids.rs`:
- Around line 119-142: The corruption guard in the offset-to-row-id mapping
needs a regression test for offsets beyond the current RowIdSequence. Extend the
relevant row-id masking test coverage around the sequence lookup in the fragment
processing flow to construct a stale physical offset with no corresponding
sequence entry, then assert the operation returns the expected internal error
and preserves the reported fragment, offset, and sequence-length details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 8e00c289-c424-4399-a206-237717db49b8
📒 Files selected for processing (2)
rust/lance/src/dataset/rowids.rsrust/lance/src/dataset/tests/dataset_overlay_index_masking.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Follow-up to #7549 (now merged).
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_idsmaps a stale overlay row's physical offset to its stable row id when building the overlay block/take set. It advanced aRowIdSequence::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 (RowIdIndexmaps ids tostart_address + positionand 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_idsalready does), skipping deleted offsets. This is also cheaper — O(stale offsets) instead of O(physical rows) — and errors (rather than silently dropping) if a selected offset has no sequence entry, since a silent drop would let a stale hit escape masking.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 overstable_row_ids) overlays fragment 1 and deletes a row below the stale offset. Fails before the fix on the stable-row-id case (age = 80leaks the stale hit), passes after; the non-stable case is a control that stays green.🤖 Generated with Claude Code