feat(compaction): compact fragments over an overlay-count limit#7772
Conversation
📝 WalkthroughWalkthroughCompaction options now support overlay-count thresholds that trigger standalone compaction. Manifest rewrites prune stale index coverage for materialized overlay fields, with tests covering threshold behavior, deletion materialization, and index reconciliation. ChangesOverlay compaction and index reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DatasetConfig
participant CompactionOptions
participant DefaultCompactionPlanner
participant Transaction
participant IndexMetadata
DatasetConfig->>CompactionOptions: parse overlay threshold
CompactionOptions->>DefaultCompactionPlanner: provide max_overlays_per_fragment
DefaultCompactionPlanner->>Transaction: rewrite fragments with excessive overlays
Transaction->>IndexMetadata: prune stale rewritten-fragment coverage
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 2
🤖 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 8074-8106: Update the id_val_map helper to use
scanner.try_into_batch() instead of try_into_stream().try_collect(), then
process the returned single RecordBatch directly while preserving the existing
id-to-value mapping behavior.
- Around line 7946-7969: Update the test helper create_base_dataset to construct
the batch with arrow_array’s record_batch! macro, replacing the manual
ArrowSchema, Arc, and RecordBatch::try_new setup. Preserve the existing id and
val columns and values, then continue using the resulting batch with
RecordBatchIterator and Dataset::write unchanged.
🪄 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: 4b224d4d-3ae2-4788-a9e7-86c9335b97f8
📒 Files selected for processing (2)
rust/lance/src/dataset/optimize.rsrust/lance/src/dataset/transaction.rs
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
This is actually surprisingly straightforward (except for the fragment bitmap pruning)
That being said, this means overlays will eventually trigger a full-row rewrite. This is true even if the overlays all affect a single field. In those cases it seems it would be more efficient to compact the overlays through some kind of data replacement operation wouldn't it?
Perhaps that is an optimization that can come later.
| /// Maximum number of data overlay files a fragment may carry before it is | ||
| /// fully compacted. When set, any fragment with more than this many overlays | ||
| /// is rewritten into a fresh fragment with its overlays (and deletions) | ||
| /// materialized into the base data, dropping the fragment from any index | ||
| /// left stale by those overlays. | ||
| /// Defaults to `Some(10)`. Set to `None` to disable the overlay-count | ||
| /// trigger entirely. |
There was a problem hiding this comment.
Can I set this to 0 to force all overlays to be folded in? If so, we should maybe mention that.
There was a problem hiding this comment.
Yes — the check is overlays.len() > max, so Some(0) marks any fragment carrying at least one overlay as CompactItself. I updated the doc comment to call that out (and 828eb7d12 also lets the manifest config set "none" to disable the trigger, since the default is now Some(10)).
| /// After a `Rewrite` fully compacts a fragment, its data overlays are baked | ||
| /// into the new fragment's base data. An index built *before* one of those | ||
| /// overlays (`overlay.committed_version > index.dataset_version`) indexed the | ||
| /// stale pre-overlay values -- and unlike a live overlay, the compacted | ||
| /// fragment no longer signals that staleness to the query path. Drop each | ||
| /// rewritten (new) fragment from the coverage of any index covering a field | ||
| /// such an overlay supplied, so those rows fall back to a flat scan. |
There was a problem hiding this comment.
I don't think you've done anything wrong, I'm just marveling at how complex this can get.
There was a problem hiding this comment.
Yeah, part of my thinks it would have been simpler if compaction and index updates were always done together. Then there's less sad intermediate states.
Add a `max_overlays_per_fragment` option to `CompactionOptions`. When a fragment carries more than this many data overlay files, it is fully compacted into a fresh fragment with its overlays (and deletions) materialized into the base data. Because the rewrite removes the overlays that mask stale index values at query time, the `Rewrite` commit drops the compacted fragment from the coverage of any index left stale by those overlays (field-aware, version-gated via `committed_version > index.dataset_version`), so an index is never left serving stale values; the rows fall back to a flat scan until reindex. Overlay->overlay merge and in-place folds are left as follow-ups. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dule Address review feedback: move the overlay-compaction tests out of a separate `overlay_compaction_tests` module into the existing `tests` module, and use the `record_batch!` macro and `try_into_batch()` test idioms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Enable the overlay-count compaction trigger by default so fragments that accumulate more than 10 data overlay files are folded back into base data without an explicit opt-in. Set to None to disable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0243b3f to
c24fee8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 270-271: Update the manifest configuration parser for the
overlay-count setting near the documented default and its parsing logic around
the referenced lines so it accepts an explicit "none" value as None, while
retaining integer parsing for enabled thresholds and rejecting invalid values.
Ensure removing the key still uses the default Some(10), and add tests covering
enabled, disabled, and invalid manifest values.
- Line 272: Avoid adding max_overlays_per_fragment directly to the public
CompactionOptions struct because downstream exhaustive struct literals would
break. Mark CompactionOptions as non-exhaustive, or expose this setting through
its existing builder/API while preserving compatibility with current struct
literals.
In `@rust/lance/src/dataset/transaction.rs`:
- Around line 2817-2821: Update handle_rewrite_fragments to resolve and retain
the fragment IDs assigned during manifest construction before reconciling
fragments and indexes, rather than rereading potentially stale IDs from
group.new_fragments. Ensure ID-0 cloned fragments use their assigned nonzero IDs
for index pruning, and add coverage for a rewrite using Fragment::new(0).
- Around line 2828-2832: Update both stale-index handling paths around is_stale
and fragment_bitmap so a stale index with unknown fragment coverage (None) is
conservatively assigned an empty bitmap or otherwise invalidated, while
retaining removal of rewritten fragment IDs for known coverage. Add a test
covering a stale index with a None fragment_bitmap and verify it cannot serve
the rewritten fragment.
🪄 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: 4b271a84-92e3-4b06-ab01-1af71f0d8a9b
📒 Files selected for processing (2)
rust/lance/src/dataset/optimize.rsrust/lance/src/dataset/transaction.rs
| /// left stale by those overlays. | ||
| /// Defaults to `Some(10)`. Set to `None` to disable the overlay-count | ||
| /// trigger entirely. | ||
| pub max_overlays_per_fragment: Option<usize>, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline rust/lance/src/dataset/optimize.rs \
--match CompactionOptions --view expanded
sed -n '185,280p' rust/lance/src/dataset/optimize.rsRepository: lance-format/lance
Length of output: 6366
🏁 Script executed:
#!/bin/bash
sed -n '180,220p' rust/lance/src/dataset/optimize.rs
printf '\n---\n'
rg -n "#\[non_exhaustive\]|pub struct CompactionOptions|impl CompactionOptions|pub use .*CompactionOptions" rust/lance/src -SRepository: lance-format/lance
Length of output: 2096
Avoid adding this public field directly CompactionOptions is public and not #[non_exhaustive], so this addition breaks downstream exhaustive struct literals. Make the type non-exhaustive or expose the setting through the existing builder/API instead.
🤖 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` at line 272, Avoid adding
max_overlays_per_fragment directly to the public CompactionOptions struct
because downstream exhaustive struct literals would break. Mark
CompactionOptions as non-exhaustive, or expose this setting through its existing
builder/API while preserving compatibility with current struct literals.
Source: Coding guidelines
| let new_fragment_ids = group | ||
| .new_fragments | ||
| .iter() | ||
| .map(|f| f.id as u32) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Prune using the fragment IDs assigned during manifest construction.
Line 2817 reads IDs from group.new_fragments, but handle_rewrite_fragments assigns an ID only to cloned fragments when the supplied ID is 0. The helper then removes ID 0, leaving the actual rewritten fragment covered by a stale index. Resolve IDs once before fragment and index reconciliation, and test a rewrite with Fragment::new(0).
Also applies to: 6483-6488
🤖 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/transaction.rs` around lines 2817 - 2821, Update
handle_rewrite_fragments to resolve and retain the fragment IDs assigned during
manifest construction before reconciling fragments and indexes, rather than
rereading potentially stale IDs from group.new_fragments. Ensure ID-0 cloned
fragments use their assigned nonzero IDs for index pruning, and add coverage for
a rewrite using Fragment::new(0).
| if is_stale && let Some(fragment_bitmap) = &mut index.fragment_bitmap { | ||
| for new_id in &new_fragment_ids { | ||
| fragment_bitmap.remove(*new_id); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Invalidate stale indices whose fragment coverage is unknown.
At Line 2828, a stale index is changed only when fragment_bitmap is Some. None means unknown coverage, so the stale index may still serve the rewritten fragment. Conservatively set an empty bitmap—or derive known coverage—and add a None-bitmap test.
Conservative fix
- if is_stale && let Some(fragment_bitmap) = &mut index.fragment_bitmap {
- for new_id in &new_fragment_ids {
- fragment_bitmap.remove(*new_id);
+ if is_stale {
+ match &mut index.fragment_bitmap {
+ Some(fragment_bitmap) => {
+ for new_id in &new_fragment_ids {
+ fragment_bitmap.remove(*new_id);
+ }
+ }
+ None => {
+ index.fragment_bitmap = Some(RoaringBitmap::new());
+ }
}
}Also applies to: 6490-6500
🤖 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/transaction.rs` around lines 2828 - 2832, Update both
stale-index handling paths around is_stale and fragment_bitmap so a stale index
with unknown fragment coverage (None) is conservatively assigned an empty bitmap
or otherwise invalidated, while retaining removal of rewritten fragment IDs for
known coverage. Add a test covering a stale index with a None fragment_bitmap
and verify it cannot serve the rewritten fragment.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The overlay-count trigger defaults to Some(10), so removing the manifest key leaves it enabled — there was no way to disable it through config. Accept an explicit "none" (case-insensitive) to set it to None, and note that Some(0) folds in every fragment carrying any overlay. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/lance/src/dataset/optimize.rs (2)
8318-8327: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse named RecordBatch columns at both test sites.
Both sites depend on projection order instead of the column contract:
rust/lance/src/dataset/optimize.rs#L8318-L8327: replace positional access withbatch["id"]andbatch["val"].rust/lance/src/dataset/optimize.rs#L8495-L8500: replacebatch.column(0)withbatch["id"].As per coding guidelines, tests should use
batch["column_name"]for RecordBatch access.🤖 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 8318 - 8327, Update both test sites in rust/lance/src/dataset/optimize.rs: at lines 8318-8327, access the RecordBatch columns by name using batch["id"] and batch["val"] instead of positional column(0)/column(1); at lines 8495-8500, replace batch.column(0) with batch["id"].Source: Coding guidelines
8378-8393: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd execution-level coverage for
Some(0)andNone.The implementation documents that
Some(0)compacts any overlaid fragment andNonedisables the trigger, but planner tests only exercise threshold2. Add tests proving both boundary behaviors end-to-end.As per coding guidelines, “All Rust bug fixes and features must include 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/optimize.rs` around lines 8378 - 8393, Add execution-level tests alongside test_below_threshold_is_a_noop covering overlay_only_options(0) and a None threshold. Verify Some(0) compacts an overlaid fragment, while None leaves the dataset unchanged and performs no compaction, using the existing metrics and fragment-overlay assertions.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/optimize.rs`:
- Around line 6544-6551: Update the invalid-value test for
CompactionOptions::from_dataset_config to capture the returned error, assert
that it matches Error::InvalidInput, and then verify the existing
max_overlays_per_fragment and not_a_number message contents.
---
Outside diff comments:
In `@rust/lance/src/dataset/optimize.rs`:
- Around line 8318-8327: Update both test sites in
rust/lance/src/dataset/optimize.rs: at lines 8318-8327, access the RecordBatch
columns by name using batch["id"] and batch["val"] instead of positional
column(0)/column(1); at lines 8495-8500, replace batch.column(0) with
batch["id"].
- Around line 8378-8393: Add execution-level tests alongside
test_below_threshold_is_a_noop covering overlay_only_options(0) and a None
threshold. Verify Some(0) compacts an overlaid fragment, while None leaves the
dataset unchanged and performs no compaction, using the existing metrics and
fragment-overlay assertions.
🪄 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: 463e2d63-156a-4bf7-a9ca-01495c8e56a6
📒 Files selected for processing (1)
rust/lance/src/dataset/optimize.rs
A minimal, first slice of overlay-aware compaction (OSS-1326), stacked on #7536 (OSS-1324, `will/oss-1324-take-can-read-overlays`) — review against that base. ## What it does Adds a `max_overlays_per_fragment: Option<usize>` option to `CompactionOptions`. When a fragment carries **more than** this many data overlay files, the planner marks it `CompactItself`, so `compact_files` fully rewrites it into a fresh fragment with its overlays **and** deletions materialized into the base data. A fragment at or below the limit is not a candidate on this basis. The read side needs no new machinery: compaction already scans each fragment through the normal read path, which (per #7536) resolves overlays and applies deletions, so the merged post-image is what gets written to the new base file. ### Index correctness A full rewrite removes the overlays, and with them the staleness signal the query path uses to mask an index older than an overlay. So the `Rewrite` commit now drops the rewritten fragment from the coverage of any index left stale by those overlays — **field-aware** (only indices covering a field an overlay actually supplied) and **version-gated** (`overlay.committed_version > index.dataset_version`). Those rows fall back to a flat scan until reindex; an index is never left serving stale values. Non-stale indices (built at/after the overlay, or on un-overlaid fields) are remapped normally. This reuses the existing `Operation::Rewrite` path — no new persisted operation or conflict-matrix change. ## Scope / follow-ups Only the "fully compact the fragment" strategy is implemented. Overlay→overlay merge, in-place overlay→base folds (column rewrite preserving row addresses), and rebuild-in-place index reconciliation are left as follow-ups. ## Tests - Planner/e2e (`optimize.rs`): overlay count over the limit triggers a full compaction (fresh single-file fragment, overlays cleared, values materialized); at-or-below limit is a no-op; deletions are materialized alongside overlays; a stale scalar index is dropped from the compacted fragment's coverage and the indexed query stays correct. - Unit (`transaction.rs`): `prune_overlay_stale_fields_from_indices` is field-aware and version-gated (stale index dropped; index at/after the overlay kept; index on an un-overlaid field untouched). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a configurable overlay-count trigger for compaction; fragments exceeding the limit are compacted into standalone form. * Default limit is 10 overlays per fragment, configurable via `lance.compaction.max_overlays_per_fragment` (set to `none` to disable). * **Bug Fixes** * Prevented stale index coverage from returning outdated values after overlay/materialization compaction. * Improved accuracy by pruning rewritten fragment coverage from older index versions, causing queries to fall back to scans where needed. * **Tests** * Added coverage for the overlay trigger and index-staleness pruning behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 6b15a8e)
2 participants
A minimal, first slice of overlay-aware compaction (OSS-1326), stacked on #7536 (OSS-1324,
will/oss-1324-take-can-read-overlays) — review against that base.What it does
Adds a
max_overlays_per_fragment: Option<usize>option toCompactionOptions. When a fragment carries more than this many data overlay files, the planner marks itCompactItself, socompact_filesfully rewrites it into a fresh fragment with its overlays and deletions materialized into the base data. A fragment at or below the limit is not a candidate on this basis.The read side needs no new machinery: compaction already scans each fragment through the normal read path, which (per #7536) resolves overlays and applies deletions, so the merged post-image is what gets written to the new base file.
Index correctness
A full rewrite removes the overlays, and with them the staleness signal the query path uses to mask an index older than an overlay. So the
Rewritecommit now drops the rewritten fragment from the coverage of any index left stale by those overlays — field-aware (only indices covering a field an overlay actually supplied) and version-gated (overlay.committed_version > index.dataset_version). Those rows fall back to a flat scan until reindex; an index is never left serving stale values. Non-stale indices (built at/after the overlay, or on un-overlaid fields) are remapped normally.This reuses the existing
Operation::Rewritepath — no new persisted operation or conflict-matrix change.Scope / follow-ups
Only the "fully compact the fragment" strategy is implemented. Overlay→overlay merge, in-place overlay→base folds (column rewrite preserving row addresses), and rebuild-in-place index reconciliation are left as follow-ups.
Tests
optimize.rs): overlay count over the limit triggers a full compaction (fresh single-file fragment, overlays cleared, values materialized); at-or-below limit is a no-op; deletions are materialized alongside overlays; a stale scalar index is dropped from the compacted fragment's coverage and the indexed query stays correct.transaction.rs):prune_overlay_stale_fields_from_indicesis field-aware and version-gated (stale index dropped; index at/after the overlay kept; index on an un-overlaid field untouched).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
lance.compaction.max_overlays_per_fragment(set tononeto disable).Bug Fixes
Tests