perf: skip fragment-reuse index for compactions with no indexed data#7774
Merged
jackye1995 merged 1 commit intoJul 14, 2026
Merged
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:
📝 WalkthroughWalkthroughDeferred compaction now detects indexed and FRI-covered fragments before writing the fragment-reuse index. Tests cover indexed, unindexed, mixed, repeated, and concurrent compaction scenarios. ChangesDeferred FRI gating
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant commit_compaction
participant DatasetIndices
participant FragReuseIndex
commit_compaction->>DatasetIndices: collect indexed fragment coverage
commit_compaction->>FragReuseIndex: include existing FRI coverage
commit_compaction->>commit_compaction: detect indexed rewrite groups
commit_compaction->>FragReuseIndex: build FRI only when indexed groups exist
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
xuanyu-z
force-pushed
the
fix/skip-fri-for-unindexed-rewrites
branch
from
0d63b27 to
607c2af
Compare
xuanyu-z
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/optimize.rs`:
- Around line 2163-2166: Add a routine debug log to the FRI skip branch
controlled by defer_index_remap and any_group_indexed, indicating that FRI
creation was skipped because no rewrite group touched indexed or chain data.
Keep the existing frag_reuse_index behavior unchanged and avoid warning-level
logging.
🪄 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: c5962f53-10b1-4e4f-bbae-1f9ede2c5fdb
📒 Files selected for processing (1)
rust/lance/src/dataset/optimize.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/optimize.rs`:
- Around line 2033-2048: The legacy index coverage expansion in
load_index_fragmaps must include the manifest’s inclusive max_fragment_id rather
than stopping before it. Update that range construction so fragment_bitmap ==
None covers the highest fragment as well, preserving the existing coverage union
in the indexed_frags calculation.
🪄 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: 412f2bf5-c3ab-473d-b5a3-800f8fb23ee5
📒 Files selected for processing (1)
rust/lance/src/dataset/optimize.rs
xuanyu-z
force-pushed
the
fix/skip-fri-for-unindexed-rewrites
branch
from
607c2af to
97f45f7
Compare
There was a problem hiding this comment.
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/optimize.rs (1)
2152-2157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStill no log on the FRI-skip path.
A prior review on this PR suggested a
debug!log whendefer_index_remapis set but no group was indexed, so operators can confirm the fix is taking effect without inspecting index metadata. That log is still absent here.♻️ Proposed addition
let frag_reuse_index = if options.defer_index_remap && any_group_indexed { Some(build_new_frag_reuse_index(dataset, frag_reuse_groups, new_fragment_bitmap).await?) } else { + if options.defer_index_remap { + debug!( + "skipping fragment-reuse index: no rewritten fragments were covered by an index" + ); + } None };As per coding guidelines, "Log warnings for silent no-op skipped operations, but omit warnings before errors because the error message is sufficient" and "use
debug!for routine/high-frequency operations."🤖 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/optimize.rs` around lines 2152 - 2157, The FRI-skip branch in the frag_reuse_index construction lacks the requested diagnostic log. Update the conditional around defer_index_remap and any_group_indexed to emit a debug! message when index remapping is deferred but no group was indexed, while preserving the existing None result and avoiding logs on error paths.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.
Outside diff comments:
In `@rust/lance/src/dataset/optimize.rs`:
- Around line 2152-2157: The FRI-skip branch in the frag_reuse_index
construction lacks the requested diagnostic log. Update the conditional around
defer_index_remap and any_group_indexed to emit a debug! message when index
remapping is deferred but no group was indexed, while preserving the existing
None result and avoiding logs on error paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 34db534c-1a2c-43d5-9b93-dcade993f22d
📒 Files selected for processing (1)
rust/lance/src/dataset/optimize.rs
xuanyu-z
force-pushed
the
fix/skip-fri-for-unindexed-rewrites
branch
2 times, most recently
from
b263afc to
44a44c7
Compare
A deferred-remap compaction records a fragment-reuse index (FRI) version for every rewrite group, even when a compaction rewrites only fragments no index covers. Such a version carries no useful remap -- no index's fragment bitmap overlaps the group's old fragments -- but it still accumulates: the cleanup trim cannot drop it while a lagging index's dataset_version sits below it, and every read pays the load-time auto-remap cost for it. On tables whose churn is dominated by not-yet-indexed data these no-op versions pile up until an index rebuild advances the index version. Decide per compaction, all-or-nothing: write the FRI only when some rewrite group's old fragments are covered by a data index, or by the existing FRI's new fragments (a not-yet-caught-up index reaches those through the composed remap chain); otherwise skip it. This is deliberately not a per-group filter -- a partial FRI is unsound because a concurrent reindex can turn a skipped "unindexed" fragment into an indexed one, and the commit conflict resolver's FRI-present path does not re-check skipped groups. Skipping entirely yields the same no-FRI state a non-deferred (inline) compaction produces, whose resolver path forces a retry if a concurrent reindex appears. Also fix an off-by-one in load_index_fragmaps: max_fragment_id is inclusive, so the fragment_bitmap==None fallback (whose coverage the skip decision reads) must include the highest fragment. Log the skip at debug. Tests: no-index and mixed (records-all-groups) cases, the chained multi-compaction case, plus the existing concurrent/cleanup-rebase tests updated to build an index (an FRI now requires one).
xuanyu-z
force-pushed
the
fix/skip-fri-for-unindexed-rewrites
branch
from
44a44c7 to
a838aac
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
A deferred-remap compaction (
defer_index_remap = true) records a fragment-reuse index (FRI) version for every rewrite group, even when a compaction rewrites only fragments that no index covers. Such a version carries no useful remap — no index'sfragment_bitmapoverlaps the group's old fragments — but it isn't free:cleanup_frag_reuse_indexcan't trim it while a lagging index'sdataset_versionsits below it, and every read pays the load-time auto-remap cost for it. On a table whose churn is dominated by not-yet-indexed data, these no-op versions accumulate (bounded only by the eventual index rebuild that advances the index version), inflating FRI metadata and per-read overhead.This is not a correctness issue — queries stay correct throughout, since scalar indexes apply the FRI at read time. It only affects metadata size and read overhead.
Change
Decide per compaction, all-or-nothing: write the FRI only when some rewrite group's old fragments are covered by
Otherwise skip the FRI version entirely.
A remap advances the index's
dataset_versionto the current version on commit, so indexed-fragment FRIs still drain via remap with no rebuild; only compactions touching no indexed data change behavior (they now write no FRI).Why not a per-group filter
A partial FRI (recording only the indexed groups) is unsound: a concurrent reindex can turn a fragment that was unindexed at the compaction's read snapshot into an indexed one, and the commit conflict resolver's FRI-present path does not re-check the skipped groups — which would leave the reindexed index referencing rewritten-away fragments. Skipping the FRI entirely produces the same no-FRI state a non-deferred (inline) compaction already produces, whose conflict-resolver path forces a retryable conflict if a concurrent reindex appears.
Also
load_index_fragmaps:max_fragment_idis inclusive, so thefragment_bitmap == Nonefallback (whose coverage this decision reads) now covers the highest fragment (0..max→0..=max).debug!so operators can confirm the fix is taking effect without inspecting index metadata.Tests
test_defer_index_remap_skips_fri_when_no_indexed_data— no data index → no FRI.test_defer_index_remap_mixed_records_all_groups— a mixed compaction records the unindexed group too (all-or-nothing; a per-group filter would drop it).test_defer_index_remap_multiple_compactions— chained FRI across successive deferred compactions.All
dataset::optimize(103) anddataset::index::frag_reuse(4) tests pass;fmt+clippyclean.Follow-up
commit_compactionloads index metadata twice (load_index_fragmaps+load_index_by_name); this could load once by threading pre-loaded indices through the helper (also touches the binning call site).Summary by CodeRabbit
Bug Fixes
Tests