fix: trim fragment-reuse index for non-covering indexes#7870
Conversation
|
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:
📝 WalkthroughWalkthroughFragment-reuse cleanup now computes coverage across the full reuse chain, including old and new fragments. Index remapping uses disjoint coverage to determine catch-up status, with tests covering deferred remapping, unrelated indexes, and fragment removal. ChangesFragment reuse cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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/index/frag_reuse.rs (1)
51-65: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueChain bitmap computation and loop integration look correct.
The union of old+new fragment IDs across all reuse versions is the right scope for the disjoint check. Including new fragments ensures deferred-remap indices (whose bitmaps were advanced onto new fragments) are not wrongly cleared. The per-version iteration with the shared
chain_frag_bitmapis efficient since the bitmap is computed once.One minor:
retained_versionsat line 60 has a known upper bound (frag_reuse_details.versions.len()). As per coding guidelines, "UseVec::with_capacity()when size is known or estimable, and prefer over-estimating capacity to multiple reallocations."♻️ Optional: pre-allocate retained_versions
- let mut retained_versions = Vec::new(); + let mut retained_versions = Vec::with_capacity(frag_reuse_details.versions.len());🤖 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/index/frag_reuse.rs` around lines 51 - 65, Pre-allocate retained_versions with capacity based on frag_reuse_details.versions.len() before iterating through the reuse versions, while leaving the existing retention logic 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.
Nitpick comments:
In `@rust/lance/src/dataset/index/frag_reuse.rs`:
- Around line 51-65: Pre-allocate retained_versions with capacity based on
frag_reuse_details.versions.len() before iterating through the reuse versions,
while leaving the existing retention logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 00f3539f-4d85-479b-b246-d5e490bf5f2f
📒 Files selected for processing (1)
rust/lance/src/dataset/index/frag_reuse.rs
5c0cfed to
786200d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rust/lance/src/dataset/index/frag_reuse.rs (1)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePreallocate
retained_versions.Its maximum size is known from
frag_reuse_details.versions.len().Suggested change
- let mut retained_versions = Vec::new(); + let mut retained_versions = Vec::with_capacity(frag_reuse_details.versions.len());As per coding guidelines,
Use Vec::with_capacity() when size is known or estimable.🤖 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/index/frag_reuse.rs` at line 49, Update the retained_versions initialization in the fragment reuse flow to use Vec::with_capacity(frag_reuse_details.versions.len()), reserving space based on the known maximum number of versions.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/index/frag_reuse.rs`:
- Around line 19-25: Expand the public API rustdoc near the reuse-version
caught-up documentation with links to the relevant reuse/index types and
methods, and add a compilable example demonstrating how to await the cleanup
operation using its actual signature. Keep the example synchronized with the
current API and preserve the existing behavioral description.
- Around line 226-251: Extend
test_caught_up_uses_fragment_coverage_not_only_version with a second reuse
version and build the reuse chain from both versions. While evaluating the first
version, add an assertion that an index with a stale version and a bitmap
containing only the later version’s new fragment is not caught up, ensuring
later-version coverage is not incorrectly treated as sufficient.
---
Nitpick comments:
In `@rust/lance/src/dataset/index/frag_reuse.rs`:
- Line 49: Update the retained_versions initialization in the fragment reuse
flow to use Vec::with_capacity(frag_reuse_details.versions.len()), reserving
space based on the known maximum number of versions.
🪄 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: 7a4b5eff-76ee-4129-8cde-32b71d5e7e83
📒 Files selected for processing (1)
rust/lance/src/dataset/index/frag_reuse.rs
786200d to
9f8640b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rust/lance/src/dataset/index/frag_reuse.rs (1)
49-49: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPreallocate
retained_versions.Its maximum size is known from
frag_reuse_details.versions.len().Proposed change
- let mut retained_versions = Vec::new(); + let mut retained_versions = Vec::with_capacity(frag_reuse_details.versions.len());As per coding guidelines, “Use
Vec::with_capacity()when size is known or estimable.”🤖 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/index/frag_reuse.rs` at line 49, Update the retained_versions initialization in the fragment reuse logic to use Vec::with_capacity with frag_reuse_details.versions.len(), preserving the existing element type and subsequent population behavior.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/index/frag_reuse.rs`:
- Line 49: Update the retained_versions initialization in the fragment reuse
logic to use Vec::with_capacity with frag_reuse_details.versions.len(),
preserving the existing element type and subsequent population behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 39b88856-f4ab-47b2-91f3-618ea5eb521d
📒 Files selected for processing (1)
rust/lance/src/dataset/index/frag_reuse.rs
`is_index_remap_caught_up` reported an index as not-caught-up whenever its `dataset_version` predated a reuse version, even when the index covers none of the fragments that version rewrote. On a table with multiple indexes this pins the fragment-reuse index forever: remapping the covering index advances only its own version, so a non-covering sibling keeps the check false, `cleanup_frag_reuse_index` retains every version, and the FRI (and the fragments it references) can never be trimmed or garbage-collected. Treat an index whose fragment bitmap is disjoint from every fragment the reuse chain touches (old and new) as caught up, regardless of its dataset_version -- it holds no row address the FRI would remap. The new-fragment side of the disjointness check is required: a deferred remap advances a covering index's bitmap onto the new fragments before its data is remapped, so checking only old fragments would wrongly clear a still-stale index and trim a version it needs. Also fixes the deletion case: a fragment removed outright (every row deleted, no replacement) empties a covering index's bitmap at commit; the disjoint check then treats it as caught up instead of pinning the version forever (its remap hits the drop-everything path and never advances the version). Tests: non-covering, covering-stale (old frag), covering-advanced-stale (new frag), remapped, and fragment-removal/emptied-index cases. Verified they fail without the fix.
9f8640b to
89a5acc
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.
🟡 Other comments (1)
rust/lance/src/dataset/index/frag_reuse.rs-19-25 (1)
19-25: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winQualify the missing-bitmap rule.
Line 24 says a missing bitmap always counts as caught up, but a stale index returns
falseat the dataset-version gate before reaching theNonebranch. State that it counts as caught up only once the index is at/past the reuse version.As per coding guidelines, “Ensure doc comments match actual semantics.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/src/dataset/index/frag_reuse.rs` around lines 19 - 25, Update the documentation for the index caught-up rule near the dataset-version gate to qualify that a missing bitmap counts as caught up only when the index is at or past the reuse version; preserve the existing stale-index behavior that returns false before evaluating the missing-bitmap branch.Source: Coding guidelines
🧹 Nitpick comments (1)
rust/lance/src/dataset/index/frag_reuse.rs (1)
64-71: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePreallocate the retained-version list.
The maximum size is known from
frag_reuse_details.versions.len().Proposed fix
- let mut retained_versions = Vec::new(); + let mut retained_versions = Vec::with_capacity(frag_reuse_details.versions.len());As per coding guidelines, “Use
Vec::with_capacity()when size is known or estimable.”🤖 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/index/frag_reuse.rs` around lines 64 - 71, Preallocate retained_versions using Vec::with_capacity with frag_reuse_details.versions.len() before iterating versions, while preserving the existing collection logic.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.
Other comments:
In `@rust/lance/src/dataset/index/frag_reuse.rs`:
- Around line 19-25: Update the documentation for the index caught-up rule near
the dataset-version gate to qualify that a missing bitmap counts as caught up
only when the index is at or past the reuse version; preserve the existing
stale-index behavior that returns false before evaluating the missing-bitmap
branch.
---
Nitpick comments:
In `@rust/lance/src/dataset/index/frag_reuse.rs`:
- Around line 64-71: Preallocate retained_versions using Vec::with_capacity with
frag_reuse_details.versions.len() before iterating versions, while preserving
the existing collection logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 7a8ccd45-7bc1-4a22-84d4-dae81841f11c
📒 Files selected for processing (1)
rust/lance/src/dataset/index/frag_reuse.rs
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
## What `is_index_remap_caught_up` (used by `cleanup_frag_reuse_index`) reports an index as *not* caught up whenever its `dataset_version` predates a fragment-reuse (FRI) version — even when the index covers none of the fragments that version rewrote. On a table with more than one index this pins the fragment-reuse index forever: - a compaction with deferred remap rewrites fragments covered by index A but not index B, and writes an FRI version; - the remap catch-up remaps A and advances A's `dataset_version`, but B covers none of those fragments, so its remap is a no-op and B's `dataset_version` never advances; - `cleanup_frag_reuse_index` retains the version because B fails the version gate, even though B needs nothing; - the FRI (and the old fragments it references) can never be trimmed or garbage-collected, and any catch-up that fires while the FRI is non-empty re-runs forever making no progress. ## Fix Treat an index whose `fragment_bitmap` is disjoint from every fragment the reuse chain touches (old **and** new) as caught up, regardless of `dataset_version`: it holds no row address the FRI would remap, so trimming can never strand it (fragment ids are never reused). The disjointness set must include the **new** fragments, not only the old ones: a deferred-remap commit advances a *covering* index's bitmap onto the new fragments before its data is remapped, so an old-frag-only check would wrongly clear a still-stale index and trim a version it needs. Indexes that intersect the chain fall through to the existing conservative logic unchanged. ## Tests Adds `test_caught_up_uses_fragment_coverage_not_only_version` covering: - non-covering index with a stale version → caught up (the bug); - covering index still holding the old fragment → not caught up; - covering index advanced onto the new fragment but not yet remapped → not caught up (why new frags are in the check); - remapped index (version advanced) → caught up; - fragment removed outright (deletion) with an emptied index → caught up (not pinned forever). Verified the test fails without the fix. Existing `frag_reuse` tests pass (`--test-threads=1`). (cherry picked from commit 4ead58c)
4 participants
What
is_index_remap_caught_up(used bycleanup_frag_reuse_index) reports an index as not caught up whenever itsdataset_versionpredates a fragment-reuse (FRI) version — even when the index covers none of the fragments that version rewrote.On a table with more than one index this pins the fragment-reuse index forever:
dataset_version, but B covers none of those fragments, so its remap is a no-op and B'sdataset_versionnever advances;cleanup_frag_reuse_indexretains the version because B fails the version gate, even though B needs nothing;Fix
Treat an index whose
fragment_bitmapis disjoint from every fragment the reuse chain touches (old and new) as caught up, regardless ofdataset_version: it holds no row address the FRI would remap, so trimming can never strand it (fragment ids are never reused).The disjointness set must include the new fragments, not only the old ones: a deferred-remap commit advances a covering index's bitmap onto the new fragments before its data is remapped, so an old-frag-only check would wrongly clear a still-stale index and trim a version it needs. Indexes that intersect the chain fall through to the existing conservative logic unchanged.
Tests
Adds
test_caught_up_uses_fragment_coverage_not_only_versioncovering:Verified the test fails without the fix. Existing
frag_reusetests pass (--test-threads=1).