fix(compaction): skip remapping when no indices exist#7778
fix(compaction): skip remapping when no indices exist#7778everySympathy wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds ChangesIndex remap gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant commit_compaction
participant IndexRemapper
participant RewriteResult
commit_compaction->>IndexRemapper: create remapper and call is_empty()
IndexRemapper-->>commit_compaction: empty or non-empty result
commit_compaction->>IndexRemapper: remap_indices() when non-empty
IndexRemapper-->>RewriteResult: rewritten indices and row addresses
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
| async fn is_empty(&self) -> Result<bool> { | ||
| if self.dataset.manifest.index_section.is_none() { | ||
| return Ok(true); | ||
| } | ||
| Ok(self.dataset.load_indices().await?.is_empty()) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Exclude fragment-reuse-only metadata from remapping eligibility.
remap_indices skips FRAG_REUSE_INDEX_NAME, but this method returns false whenever that is the only loaded index. Compaction then builds a row-address map no remapper can consume. Base this result on whether any loaded index satisfies the same eligibility predicate, and add an FRI-only regression test.
Proposed fix
- Ok(self.dataset.load_indices().await?.is_empty())
+ let indices = self.dataset.load_indices().await?;
+ Ok(!indices
+ .iter()
+ .any(|index| index.name != FRAG_REUSE_INDEX_NAME))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fn is_empty(&self) -> Result<bool> { | |
| if self.dataset.manifest.index_section.is_none() { | |
| return Ok(true); | |
| } | |
| Ok(self.dataset.load_indices().await?.is_empty()) | |
| } | |
| async fn is_empty(&self) -> Result<bool> { | |
| if self.dataset.manifest.index_section.is_none() { | |
| return Ok(true); | |
| } | |
| let indices = self.dataset.load_indices().await?; | |
| Ok(!indices | |
| .iter() | |
| .any(|index| index.name != FRAG_REUSE_INDEX_NAME)) | |
| } |
🤖 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.rs` around lines 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.
| #[derive(Debug, Default, Clone, PartialEq, Serialize, Deserialize)] | ||
| struct EmptyRemap {} | ||
|
|
||
| #[async_trait] | ||
| impl IndexRemapper for EmptyRemap { | ||
| async fn is_empty(&self) -> Result<bool> { | ||
| Ok(true) | ||
| } | ||
|
|
||
| async fn remap_indices(&self, _: RowAddrRemap, _: &[u64]) -> Result<Vec<RemappedIndex>> { | ||
| panic!("remap_indices should not be called for an empty remapper") | ||
| } | ||
| } | ||
|
|
||
| impl IndexRemapperOptions for EmptyRemap { | ||
| fn create_remapper(&self, _: &Dataset) -> Result<Box<dyn IndexRemapper>> { | ||
| Ok(Box::new(Self {})) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Exclude the new test double from coverage.
Mark EmptyRemap and its test-only implementations with #[cfg_attr(coverage, coverage(off))]. As per coding guidelines, “Exclude test utilities from coverage with #[cfg_attr(coverage, coverage(off))].”
🤖 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 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.
Source: Coding guidelines
| /// Return whether there are no indices that need row address remapping. | ||
| /// | ||
| /// The default is conservative for custom remappers that do not expose | ||
| /// their index inventory. | ||
| async fn is_empty(&self) -> Result<bool> { | ||
| Ok(false) | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the new public hook with an example and API link.
Add a usage example and link is_empty to remap_indices/the compaction behavior. As per coding guidelines, “Document all public APIs with examples and links to relevant structs and methods.”
🤖 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 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.
Source: Coding guidelines
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
What changed
IndexRemapper::is_emptyhook whose default keeps the existing behavior for custom remappers.Why
For datasets without stable row IDs, compaction currently builds an old-to-new row-address map before loading the index inventory. With no indices, the map is discarded without being consumed. In direct remap mode this is an O(number of rewritten rows)
HashMap, so a large distributed compaction can exhaust the commit process memory even though the dataset has no indices.The change only skips remapping when the configured remapper explicitly reports that it is empty. Indexed datasets and third-party remappers retain the existing behavior.
Validation
cargo fmt --all -- --checkcargo test -p lance test_commit_skips_empty_index_remappercargo test -p lance test_compact_distributedcargo test -p lance test_btree_index_remap_after_compactioncargo clippy --all --tests --benches -- -D warningsSummary by CodeRabbit
Performance Improvements
Bug Fixes