perf(compaction): skip building row-address maps when index remapping is not needed#7778
perf(compaction): skip building row-address maps when index remapping is not needed#7778everySympathy wants to merge 1 commit into
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:
📝 WalkthroughWalkthroughMakes index remapper creation asynchronous and optional, caches index metadata, and gates compaction row-address capture and index rewriting on the presence of remappable indices. ChangesIndex remap gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant commit_compaction
participant DatasetIndexRemapperOptions
participant DatasetIndexRemapper
participant RewriteResult
commit_compaction->>DatasetIndexRemapperOptions: create_remapper().await
DatasetIndexRemapperOptions->>DatasetIndexRemapper: preload index metadata
DatasetIndexRemapper-->>commit_compaction: Some(remapper) or None
commit_compaction->>DatasetIndexRemapper: remap_indices() when Some
DatasetIndexRemapper-->>RewriteResult: rewritten indices
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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.
Actionable comments posted: 3
🤖 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.rs`:
- Around line 59-64: Update DatasetIndex::is_empty to evaluate loaded indices
using the same eligibility predicate as remap_indices, excluding
FRAG_REUSE_INDEX_NAME, rather than checking whether any index exists. Return
true when the manifest has no indices or only fragment-reuse metadata, and add a
regression test covering the FRI-only case.
In `@rust/lance/src/dataset/optimize.rs`:
- Around line 2964-2982: Exclude the EmptyRemap test double from coverage by
adding #[cfg_attr(coverage, coverage(off))] to the EmptyRemap type and its
test-only IndexRemapper and IndexRemapperOptions implementations.
In `@rust/lance/src/dataset/optimize/remapping.rs`:
- Around line 53-60: Expand the documentation for the public Remapper::is_empty
hook with a concise usage example and intra-document links to remap_indices and
the compaction behavior it controls. Preserve the existing conservative default
and explain how custom remappers should report whether indices require
row-address remapping.
🪄 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: 2a48977e-7eed-4767-b32f-b17312141264
📒 Files selected for processing (3)
rust/lance/src/dataset/index.rsrust/lance/src/dataset/optimize.rsrust/lance/src/dataset/optimize/remapping.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
wjones127
left a comment
wjones127
left a comment
There was a problem hiding this comment.
This is a good fix, but I'd like you to explore a simpler solution before we accept this change.
| let remapper = remap_options.create_remapper(dataset)?; | ||
| if remapper.is_empty().await? { |
There was a problem hiding this comment.
suggestion: What if instead of adding a new is_empty(), we just made create_remapper return Option<_>? I think that would be simpler and avoid needing to add another trait method.
There was a problem hiding this comment.
Thanks — I’ve updated this to remove is_empty() and make create_remapper return Result<Option<Box<dyn IndexRemapper>>>, as suggested. To correctly return None for the FRI-only case before materializing the row-address map, create_remapper now needs to load index metadata, so I also made it async.
One caveat is that IndexRemapperOptions is a public trait, so changing both the return type and the method to async is a source-breaking API change for external implementors and direct callers. Are you comfortable with that trade-off, or would you prefer a compatibility-preserving approach?
c4a33e2 to
3beb055
Compare
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 8231-8565: Extract the overlay-compaction helpers, imports, and
tests beginning with create_base_dataset into a dedicated submodule located
beside binary_copy. Keep all overlay-specific use declarations inside that
submodule at its top, preserve the existing test behavior and symbols, and
remove the block from the parent module.
In `@rust/lance/src/dataset/optimize/remapping.rs`:
- Around line 64-70: Preserve the existing synchronous required factory method
on IndexRemapperOptions returning Box<dyn IndexRemapper>. Add a separate async
optional method with a default implementation that calls the existing factory
and wraps its result in Some, then update only the metadata-aware implementation
to override the optional method and return None when no remappable indices
exist.
🪄 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: 1cef1c44-f8c7-44e8-ad01-5bed51c0f29d
📒 Files selected for processing (3)
rust/lance/src/dataset/index.rsrust/lance/src/dataset/optimize.rsrust/lance/src/dataset/optimize/remapping.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 8231-8565: Extract the overlay-compaction helpers, imports, and
tests beginning with create_base_dataset into a dedicated submodule located
beside binary_copy. Keep all overlay-specific use declarations inside that
submodule at its top, preserve the existing test behavior and symbols, and
remove the block from the parent module.
In `@rust/lance/src/dataset/optimize/remapping.rs`:
- Around line 64-70: Preserve the existing synchronous required factory method
on IndexRemapperOptions returning Box<dyn IndexRemapper>. Add a separate async
optional method with a default implementation that calls the existing factory
and wraps its result in Some, then update only the metadata-aware implementation
to override the optional method and return None when no remappable indices
exist.
🪄 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: 1cef1c44-f8c7-44e8-ad01-5bed51c0f29d
📒 Files selected for processing (3)
rust/lance/src/dataset/index.rsrust/lance/src/dataset/optimize.rsrust/lance/src/dataset/optimize/remapping.rs
🛑 Comments failed to post (2)
rust/lance/src/dataset/optimize.rs (1)
8231-8565: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Extract the overlay-compaction tests into a dedicated submodule.
This adds over 300 lines of helpers, imports, and tests to an already large module. Move the block beside
binary_copyso its imports remain at the submodule top and the feature is independently maintainable.As per coding guidelines, “Place
useimports at the top of the file” and “Extract substantial new logic into dedicated submodules instead of inlining it into large files.” <coding_guidelines>🤖 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 8231 - 8565, Extract the overlay-compaction helpers, imports, and tests beginning with create_base_dataset into a dedicated submodule located beside binary_copy. Keep all overlay-specific use declarations inside that submodule at its top, preserve the existing test behavior and symbols, and remove the block from the parent module.Source: Coding guidelines
rust/lance/src/dataset/optimize/remapping.rs (1)
64-70: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve the existing factory method for third-party remappers.
Changing this required trait method from synchronous
Box<_>to asynchronousOption<Box<_>>breaks every external implementation. Add a new async optional method with a default that wraps the existing factory inSome, then override it only where metadata-aware gating is needed.As per coding guidelines, “Do not break public API signatures; deprecate old APIs and add a replacement.” <coding_guidelines>
🤖 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/remapping.rs` around lines 64 - 70, Preserve the existing synchronous required factory method on IndexRemapperOptions returning Box<dyn IndexRemapper>. Add a separate async optional method with a default implementation that calls the existing factory and wraps its result in Some, then update only the metadata-aware implementation to override the optional method and return None when no remappable indices exist.Source: Coding guidelines
29f80c1 to
dc6b17b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.rs`:
- Around line 211-212: Replace the temporary-directory test URI in
rust/lance/src/dataset/index.rs lines 211-212 with plain memory://, removing the
test_dir and test_uri setup. In rust/lance/src/dataset/optimize.rs lines 3012
and 3065, replace each TempStrDir-based URI with plain memory://; no counters or
unique suffixes are needed.
In `@rust/lance/src/dataset/optimize.rs`:
- Around line 2027-2034: The remapper eligibility used by commit_compaction must
match the planning snapshot captured by rewrite_files. Prevent commit-time
changes to index state from creating a remapper when planned tasks lack
row_addrs, either by rejecting the mismatch or by carrying and using the
planning snapshot through commit; preserve remapping for tasks planned with
remappable indices.
In `@rust/lance/src/dataset/optimize/remapping.rs`:
- Around line 64-70: Preserve the existing public
IndexRemapperOptions::create_remapper signature for downstream implementors and
callers, marking it deprecated if needed. Add a separate async optional remapper
method with a default implementation that delegates to the compatibility hook,
and update internal compaction code to use the new method without breaking
existing trait implementations.
🪄 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: 8fab149c-ca20-44c5-ac98-1592a886caea
📒 Files selected for processing (3)
rust/lance/src/dataset/index.rsrust/lance/src/dataset/optimize.rsrust/lance/src/dataset/optimize/remapping.rs
| #[async_trait] | ||
| pub trait IndexRemapperOptions: Send + Sync { | ||
| fn create_remapper(&self, dataset: &Dataset) -> Result<Box<dyn IndexRemapper>>; | ||
| /// Creates a remapper when the dataset has indices that need row address remapping. | ||
| /// | ||
| /// Returns `None` when no remappable indices exist, allowing compaction to avoid | ||
| /// materializing an unused row address map. | ||
| async fn create_remapper(&self, dataset: &Dataset) -> Result<Option<Box<dyn IndexRemapper>>>; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline rust/lance/src/dataset/optimize/remapping.rs --items all
rg -n -C3 'IndexRemapperOptions|create_remapper' rustRepository: lance-format/lance
Length of output: 18111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' rust/lance/src/dataset/optimize/remapping.rs
printf '\n--- optimize.rs export ---\n'
sed -n '120,150p' rust/lance/src/dataset/optimize.rs
printf '\n--- index.rs impl ---\n'
sed -n '1,70p' rust/lance/src/dataset/index.rs
printf '\n--- search deprecated/compat hook ---\n'
rg -n -C2 'deprecated|compat|create_remapper|IndexRemapperOptions' rust/lance/src/datasetRepository: lance-format/lance
Length of output: 50376
Preserve the existing remapper trait method for downstream implementations.
This public trait is re-exported and implemented outside this module; changing create_remapper to async and Option<Box<_>> is a source break for external implementors and callers. Keep the current method as a deprecated compatibility hook and add a new async optional method with a default wrapper, or introduce a new trait.
🤖 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/remapping.rs` around lines 64 - 70, Preserve
the existing public IndexRemapperOptions::create_remapper signature for
downstream implementors and callers, marking it deprecated if needed. Add a
separate async optional remapper method with a default implementation that
delegates to the compatibility hook, and update internal compaction code to use
the new method without breaking existing trait implementations.
Source: Coding guidelines
dc6b17b to
c0abd7b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
rust/lance/src/dataset/optimize.rs (1)
2027-2039: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReject mixed row-address results before inline remapping.
Line 2027 treats one
row_addrspayload as sufficient. If non-empty tasks were executed against different index snapshots, lines 2112-2151 omit some tasks fromRowAddrRemap, but lines 2179-2189 remap indices for every affected fragment. This can persist incomplete index mappings. Carry remapping eligibility in the task snapshot, or reject/retry when an inline remapper is available and any rewritten task lacksrow_addrs; add a mixed-result regression test.🤖 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 2027 - 2039, The optimization flow must reject or retry mixed row-address results before inline remapping: do not let has_address_style derive solely from any task containing row_addrs when other rewritten tasks lack them. Track remapping eligibility with each task snapshot or validate completed_tasks before remap_options.create_remapper, ensuring an available inline remapper cannot process a mixed result; add a regression test covering this case.
🤖 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.
Duplicate comments:
In `@rust/lance/src/dataset/optimize.rs`:
- Around line 2027-2039: The optimization flow must reject or retry mixed
row-address results before inline remapping: do not let has_address_style derive
solely from any task containing row_addrs when other rewritten tasks lack them.
Track remapping eligibility with each task snapshot or validate completed_tasks
before remap_options.create_remapper, ensuring an available inline remapper
cannot process a mixed result; add a regression test covering this case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 365918e6-ed62-4a9b-9f63-62691dcd6278
📒 Files selected for processing (2)
rust/lance/src/dataset/index.rsrust/lance/src/dataset/optimize.rs
c0abd7b to
2f247cf
Compare
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/index.rs`:
- Around line 49-53: Preserve the existing synchronous
IndexRemapperOptions::create_remapper API for external implementors and direct
callers, marking it deprecated rather than changing its signature. Add the async
method that returns an optional remapper through a compatible new method or
extension trait, implement it for DatasetIndexRemapperOptions, and migrate only
internal compaction callers to the new async path.
🪄 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: 4f9dedd3-6db1-43d5-bb93-01d7ea1bb7c1
📒 Files selected for processing (2)
rust/lance/src/dataset/index.rsrust/lance/src/dataset/optimize.rs
2f247cf to
5425cb9
Compare
everySympathy
changed the title
everySympathy
changed the title
everySympathy
changed the title
… is not needed Avoid collecting row addresses in workers and materializing commit-side remap maps when no data indices need remapping. Reuse the loaded index metadata snapshot and exclude system indices from remapping. Co-authored-by: Yue Zhang <69956021+zhangyue19921010@users.noreply.github.com>
9842844 to
4053082
Compare
2 participants
What changed
_rowidcolumn, building an O(number of rewritten rows)RoaringTreemap, and serializing/transferring it to the commit process.IndexRemapperOptions::create_remapperasync and returnResult<Option<Box<dyn IndexRemapper>>>.Nonewhen the dataset has no remappable index metadata, including datasets with only FRI or other system indices.HashMapduring commit.Why
For datasets without stable row IDs, compaction could previously perform two layers of work before discovering that no index could consume the result:
RoaringTreemapand serialized it.HashMap.Both structures scale with the number of rewritten rows and are unnecessary when no data index needs remapping. On large distributed compactions, either layer can create avoidable memory pressure or OOM.
This PR skips the work at the earliest safe point and retains the commit-side guard for completed tasks that already contain row addresses.
Compatibility note
IndexRemapperOptionsis a public trait. This intentionally changescreate_remapperfrom:to:
This is a source-breaking API change for external trait implementors and direct callers: implementations must become async, callers must await the result, and both must handle
Option. This trade-off has been discussed with a project collaborator.Related work and attribution
This incorporates the worker-side optimization from #7773 after coordination with its author. Yue Zhang's contribution is preserved in commit with:
Thanks @zhangyue19921010 for the original implementation and collaboration.
Validation
cargo fmt --all -- --checkcargo test -p lance dataset::index::tests::test_remapper_not_created_without_remappable_indices --lib -- --exactcargo test -p lance dataset::optimize::tests::test_row_addrs_only_used_with_remappable_index --libcargo test -p lance dataset::optimize::tests::test_defer_index_remap --lib -- --exact