perf(compaction): skip capturing row addresses when no index to remap#7773
perf(compaction): skip capturing row addresses when no index to remap#7773zhangyue19921010 wants to merge 8 commits 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:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCompaction now gates row-address capture on applicable index-remapping paths and requires captured address data before commit-time remapping. Tests cover datasets without non-system indices and align deferred-remap comparison setup. ChangesCompaction row-address remapping
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
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: 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 2998-3040: The test test_skip_rowid_capture_when_no_index should
use named rstest cases via #[case::...], replacing #[values(false, true)] so
failures identify the indexed and unindexed scenarios. Strengthen the result
assertions by verifying row_addrs is absent without an index and present with an
index containing a non-empty serialized mapping, rather than checking only
is_some().
🪄 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: 855d2ac3-ab4f-4861-b5bd-fa42617a7701
📒 Files selected for processing (1)
rust/lance/src/dataset/optimize.rs
There was a problem hiding this comment.
Actionable comments posted: 2
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)
3370-3379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
create_scalar_indexfordataset2.This block duplicates the helper introduced at Lines 2339-2350. Calling
create_scalar_index(&mut dataset2, "i", false).awaitkeeps the two comparison datasets consistent and avoids future drift.As per coding guidelines: extract logic repeated in two or more places into a shared helper.
🤖 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 3370 - 3379, Replace the inline dataset2.create_index call with the existing create_scalar_index helper, passing &mut dataset2, "i", and false, then await its result. Keep the comparison dataset setup otherwise 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.
Inline comments:
In `@rust/lance/src/dataset/optimize.rs`:
- Around line 3768-3779: Extend the FRI verification in the test around
recorded_old to also iterate over indexed and assert that every indexed fragment
is present in recorded_old, using the same all-or-nothing assertion behavior as
the existing unindexed_frags check.
- Around line 1466-1471: Update the fragment range construction in the dataset
optimization flow to replace the unchecked max_fragment_id increment with
checked_add(1). Propagate a contextual error when the maximum fragment ID cannot
be incremented, while preserving the empty-range behavior for None and the
inclusive-to-exclusive range semantics for valid IDs.
---
Outside diff comments:
In `@rust/lance/src/dataset/optimize.rs`:
- Around line 3370-3379: Replace the inline dataset2.create_index call with the
existing create_scalar_index helper, passing &mut dataset2, "i", and false, then
await its result. Keep the comparison dataset setup otherwise 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: be579c85-d5b2-436b-8b1d-261c7317f97a
📒 Files selected for processing (1)
rust/lance/src/dataset/optimize.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
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)
3370-3379: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
create_scalar_indexfordataset2.This block duplicates the helper introduced at Lines 2339-2350. Calling
create_scalar_index(&mut dataset2, "i", false).awaitkeeps the two comparison datasets consistent and avoids future drift.As per coding guidelines: extract logic repeated in two or more places into a shared helper.
🤖 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 3370 - 3379, Replace the inline dataset2.create_index call with the existing create_scalar_index helper, passing &mut dataset2, "i", and false, then await its result. Keep the comparison dataset setup otherwise 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.
Inline comments:
In `@rust/lance/src/dataset/optimize.rs`:
- Around line 3768-3779: Extend the FRI verification in the test around
recorded_old to also iterate over indexed and assert that every indexed fragment
is present in recorded_old, using the same all-or-nothing assertion behavior as
the existing unindexed_frags check.
- Around line 1466-1471: Update the fragment range construction in the dataset
optimization flow to replace the unchecked max_fragment_id increment with
checked_add(1). Propagate a contextual error when the maximum fragment ID cannot
be incremented, while preserving the empty-range behavior for None and the
inclusive-to-exclusive range semantics for valid IDs.
---
Outside diff comments:
In `@rust/lance/src/dataset/optimize.rs`:
- Around line 3370-3379: Replace the inline dataset2.create_index call with the
existing create_scalar_index helper, passing &mut dataset2, "i", and false, then
await its result. Keep the comparison dataset setup otherwise 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: be579c85-d5b2-436b-8b1d-261c7317f97a
📒 Files selected for processing (1)
rust/lance/src/dataset/optimize.rs
🛑 Comments failed to post (2)
rust/lance/src/dataset/optimize.rs (2)
1466-1471: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1435,1495p' rust/lance/src/dataset/optimize.rs printf '\n--- SEARCH max_fragment_id ---\n' rg -n "max_fragment_id" rust/lance -SRepository: lance-format/lance
Length of output: 8578
🏁 Script executed:
#!/bin/bash set -euo pipefail printf 'optimize.rs slice\n' sed -n '1458,1476p' rust/lance/src/dataset/optimize.rs printf '\nmanifest definition candidates\n' rg -n "max_fragment_id|struct Manifest|enum Manifest" rust/lance -SRepository: lance-format/lance
Length of output: 7081
🏁 Script executed:
#!/bin/bash set -euo pipefail printf 'manifest field/type definitions\n' rg -n "max_fragment_id" rust/lance/src/dataset.rs rust/lance/src/dataset/transaction.rs rust/lance/src -S printf '\n--- dataset.rs around manifest definition ---\n' sed -n '1,220p' rust/lance/src/dataset.rs printf '\n--- transaction.rs around relevant update logic ---\n' sed -n '2620,2665p' rust/lance/src/dataset/transaction.rsRepository: lance-format/lance
Length of output: 16604
Guard the fragment upper bound conversion.
m + 1can overflow at the maximum fragment id and drop the last fragment from the bitmap; usechecked_add(1)and return a contextual error 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` around lines 1466 - 1471, Update the fragment range construction in the dataset optimization flow to replace the unchecked max_fragment_id increment with checked_add(1). Propagate a contextual error when the maximum fragment ID cannot be incremented, while preserving the empty-range behavior for None and the inclusive-to-exclusive range semantics for valid IDs.Source: Coding guidelines
3768-3779: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert that indexed fragments are retained too.
The test only verifies that
unindexed_fragsappear in the FRI. It would still pass if the indexed group were accidentally omitted, so also assert that every fragment inindexedis recorded.Suggested assertion
+ for fragment_id in indexed { + assert!( + recorded_old.contains(&(fragment_id as u64)), + "indexed fragment {fragment_id} must be recorded in the FRI" + ); + }📝 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.let recorded_old: HashSet<u64> = details .versions .iter() .flat_map(|v| v.old_frag_ids()) .collect(); for f in &unindexed_frags { assert!( recorded_old.contains(f), "unindexed fragment {f} must be recorded in the FRI (all-or-nothing)" ); } for fragment_id in indexed { assert!( recorded_old.contains(&(fragment_id as u64)), "indexed fragment {fragment_id} must be recorded in the FRI" ); } }🤖 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 3768 - 3779, Extend the FRI verification in the test around recorded_old to also iterate over indexed and assert that every indexed fragment is present in recorded_old, using the same all-or-nothing assertion behavior as the existing unindexed_frags check.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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/optimize.rs`:
- Around line 8396-8404: Update the new tests’ RecordBatch access in the
affected blocks around the Int32Array downcasts to use schema-name indexing,
replacing positional batch.column(0) and batch.column(1) with batch["id"] and
batch["val"] respectively; preserve the existing downcast and assertions.
- Around line 265-273: Add a compiling documentation example to the public
max_overlays_per_fragment option, showing its actual configuration usage and
signature. Link the documentation to the relevant planner and configuration
structs or methods that consume max_overlays_per_fragment, while preserving the
existing semantics and defaults.
- Around line 6622-6628: Update the CompactionOptions::from_dataset_config test
to inspect the returned error’s concrete variant before checking its message.
Assert the expected configuration/parsing error classification and retain the
existing checks for “max_overlays_per_fragment” and “not_a_number” in the
formatted message.
🪄 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: 9c3fd3c2-8390-444e-8b7c-e2c95d934ade
📒 Files selected for processing (1)
rust/lance/src/dataset/optimize.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: 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/optimize.rs`:
- Around line 8396-8404: Update the new tests’ RecordBatch access in the
affected blocks around the Int32Array downcasts to use schema-name indexing,
replacing positional batch.column(0) and batch.column(1) with batch["id"] and
batch["val"] respectively; preserve the existing downcast and assertions.
- Around line 265-273: Add a compiling documentation example to the public
max_overlays_per_fragment option, showing its actual configuration usage and
signature. Link the documentation to the relevant planner and configuration
structs or methods that consume max_overlays_per_fragment, while preserving the
existing semantics and defaults.
- Around line 6622-6628: Update the CompactionOptions::from_dataset_config test
to inspect the returned error’s concrete variant before checking its message.
Assert the expected configuration/parsing error classification and retain the
existing checks for “max_overlays_per_fragment” and “not_a_number” in the
formatted message.
🪄 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: 9c3fd3c2-8390-444e-8b7c-e2c95d934ade
📒 Files selected for processing (1)
rust/lance/src/dataset/optimize.rs
🛑 Comments failed to post (3)
rust/lance/src/dataset/optimize.rs (3)
265-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a public-API usage example and relevant links.
The new public option documents its semantics but not a compiling usage example or links to the planner/configuration APIs that consume it.
As per coding guidelines: “
**/*.{rs,md,rst}: Document all public APIs with examples and links to relevant structs and methods; keep examples synchronized with actual signatures.”🤖 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 265 - 273, Add a compiling documentation example to the public max_overlays_per_fragment option, showing its actual configuration usage and signature. Link the documentation to the relevant planner and configuration structs or methods that consume max_overlays_per_fragment, while preserving the existing semantics and defaults.Source: Coding guidelines
6622-6628: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Assert the error variant as well as its message.
This test only checks formatted text, so it could pass with the wrong error classification.
As per coding guidelines: “Assert on both the error variant and the message content in tests; do not check only
is_err().”Proposed test update
- let err_msg = CompactionOptions::from_dataset_config(&config) - .unwrap_err() - .to_string(); + let err = CompactionOptions::from_dataset_config(&config).unwrap_err(); + assert!(matches!(&err, Error::InvalidInput { .. })); + let err_msg = err.to_string();📝 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.// Anything else is rejected. let config = HashMap::from([(key, "not_a_number".to_string())]); let err = CompactionOptions::from_dataset_config(&config).unwrap_err(); assert!(matches!(&err, Error::InvalidInput { .. })); let err_msg = err.to_string(); assert!(err_msg.contains("max_overlays_per_fragment")); assert!(err_msg.contains("not_a_number"));🤖 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 6622 - 6628, Update the CompactionOptions::from_dataset_config test to inspect the returned error’s concrete variant before checking its message. Assert the expected configuration/parsing error classification and retain the existing checks for “max_overlays_per_fragment” and “not_a_number” in the formatted message.Source: Coding guidelines
8396-8404: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use schema-name access in the new tests.
The tests use positional
batch.column(0/1)access. Usebatch["id"]andbatch["val"]instead; positional access can silently target the wrong field after schema or projection changes.As per coding guidelines: “Use
column_by_name()forRecordBatchcolumn access in production code; usebatch["column_name"]in tests.”Proposed test update
- let ids = batch - .column(0) - .as_any() - .downcast_ref::<Int32Array>() - .unwrap(); - let vals = batch - .column(1) - .as_any() - .downcast_ref::<Int32Array>() - .unwrap(); + let ids = batch["id"].as_primitive::<Int32Type>(); + let vals = batch["val"].as_primitive::<Int32Type>(); - let ids = batch - .column(0) - .as_any() - .downcast_ref::<Int32Array>() - .unwrap(); + let ids = batch["id"].as_primitive::<Int32Type>();Also applies to: 8573-8578
🤖 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 8396 - 8404, Update the new tests’ RecordBatch access in the affected blocks around the Int32Array downcasts to use schema-name indexing, replacing positional batch.column(0) and batch.column(1) with batch["id"] and batch["val"] respectively; preserve the existing downcast and assertions.Source: Coding guidelines
|
Same PR related to #7778 Closing this one |
…g is not needed (#7778) ## What changed - Skip capturing row addresses in compaction workers when the dataset has no remappable data index. - This avoids scanning the `_rowid` column, building an O(number of rewritten rows) `RoaringTreemap`, and serializing/transferring it to the commit process. - Deferred index remapping still captures row addresses because the fragment-reuse index (FRI) consumes them. - Make `IndexRemapperOptions::create_remapper` async and return `Result<Option<Box<dyn IndexRemapper>>>`. - Return `None` when the dataset has no remappable index metadata, including datasets with only FRI or other system indices. - Create the remapper before materializing the potentially large old-to-new row-address `HashMap` during commit. - Reuse the index metadata snapshot loaded during remapper creation, while excluding system indices from actual remapping. - Add regression coverage for: - datasets with no indices; - datasets with only system/FRI metadata; - datasets with a normal data index; - commit-time skipping when no remapper exists; - deferred versus immediate remapping under equivalent indexed conditions. ## Why For datasets without stable row IDs, compaction could previously perform two layers of work before discovering that no index could consume the result: 1. Workers captured every old row address into a `RoaringTreemap` and serialized it. 2. The commit process expanded those addresses into an old-to-new `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. ## Breaking change BREAKING CHANGE: `IndexRemapperOptions::create_remapper` is now async and returns `Result<Option<Box<dyn IndexRemapper>>>`. External implementations must update the method signature, callers must await the result, and both must handle the `None` case. `IndexRemapperOptions` is a public trait. This intentionally changes `create_remapper` from: ```rust fn create_remapper(&self, dataset: &Dataset) -> Result<Box<dyn IndexRemapper>>; ``` to: ```rust async fn create_remapper( &self, dataset: &Dataset, ) -> Result<Option<Box<dyn IndexRemapper>>>; ``` 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: ``` Co-authored-by: Yue Zhang <69956021+zhangyue19921010@users.noreply.github.com> ``` Thanks @zhangyue19921010 for the original implementation and collaboration. ## Validation - `cargo fmt --all -- --check` - `cargo test -p lance dataset::index::tests::test_remapper_not_created_without_remappable_indices --lib -- --exact` - `cargo test -p lance dataset::optimize::tests::test_row_addrs_only_used_with_remappable_index --lib` - `cargo test -p lance dataset::optimize::tests::test_defer_index_remap --lib -- --exact` Co-authored-by: wangzheyan <wangzheyan@bytedance.com> Co-authored-by: Yue Zhang <69956021+zhangyue19921010@users.noreply.github.com>
1 participant
What
Compaction on datasets with address-style row ids always added the
_rowidcolumn to the scan and collected every rewritten row address into anO(rows)RoaringTreemap, then serialized it back in theRewriteResult— even when there were no indices to remap. That work is pure waste in the no-index case.Change
Capture row addresses only when needed like remap action.
Impact
For no-index legacy compactions this removes an extra
_rowidscan column, anO(rows)in-memory treemap, and its serialization/transfer to the coordinator. Behavior is unchanged for indexed,defer_index_remap, and stable-row-id datasets. Concurrent index creation stays safe.Summary by CodeRabbit
Bug Fixes
Tests