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 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ 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! |
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