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:
📝 WalkthroughWalkthroughAdds a new crate-internal B‑Tree packing/planning module and integrates it across B‑tree and DiskTree: introduces packed-node space estimation, pack/plan APIs, tightens node helper visibility, restructures B‑Tree submodules and re-exports, rewrites DiskTree rewrite/packing to use deferred materialization and the new packing helpers, and updates callsites/tests to the new APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant DiskTree
participant BTreeAlgo
participant BTreeNode
participant MemTree
DiskTree->>BTreeAlgo: request plan(entries, lower/upper fences, min_slots)
BTreeAlgo->>BTreeNode: estimate packed space with fences & prefix logic
BTreeNode-->>BTreeAlgo: space estimation result
BTreeAlgo->>DiskTree: return plan (Full / Partial{right_count} / FenceOutOfSpace / NoProgress)
alt Full or Partial
DiskTree->>BTreeAlgo: request pack(KnownFenceNodeParams, ranges/entries)
BTreeAlgo->>BTreeNode: init boxed node and copy slots (pack ranges)
BTreeNode-->>DiskTree: packed node box
DiskTree->>MemTree: apply merge/purge or update separator
else FenceOutOfSpace / NoProgress
DiskTree->>MemTree: handle skip/abandon or defer
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #566 +/- ##
==========================================
+ Coverage 91.72% 92.18% +0.45%
==========================================
Files 102 103 +1
Lines 56084 57525 +1441
==========================================
+ Hits 51445 53029 +1584
+ Misses 4639 4496 -143 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
doradb-storage/src/index/btree/algo.rs (2)
451-475: Test name vs. covered scenario is slightly misleading.
test_pack_sibling_node_accounts_for_upper_fence_capacityactually exercises the path where the oversizedlower_fencealone exhausts the page andrightmost_count == 0/min_slots > 0forces the finite-fence loop to bail out withInvalidArgument. The upper fence never gets considered (loop range1..1is empty). Consider renaming to reflect the actual scenario (e.g.test_pack_sibling_node_rejects_when_fences_exceed_page) or extending the test to add a second entry so the upper-fence capacity path is genuinely exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doradb-storage/src/index/btree/algo.rs` around lines 451 - 475, The test name misrepresents what it covers: update the test function test_pack_sibling_node_accounts_for_upper_fence_capacity to either (A) rename it to reflect the actual behavior (e.g. test_pack_sibling_node_rejects_when_fences_exceed_page) or (B) change the setup so the upper-fence path is exercised by adding a second entry (create a second PackedNodeEntry and adjust entries and assertions) so that pack_sibling_node's finite-fence loop (the branch that considers the upper fence) is actually entered; keep references to pack_sibling_node, PackedNodeParams, lower_fence/rightmost_count/min_slots and assert the expected Error::InvalidArgument outcome when appropriate.
130-172: Minor:fences_fitis conservative, and one branch appears unreachable.Two observations on the inner loop:
fences_fit(viaSpaceEstimation::new) countsprefix_lenbytes unconditionally, whereasPackedNodeSpace::total_space()correctly accounts for the inline threshold viacommon_prefix_body_space, which returns 0 whenprefix_len <= INLINE_PREFIX_LEN(16 bytes). When the prefix is inline,fences_fitcan over-estimate by up to 16 bytes and trigger the early break at line 137 sooner than necessary. SincePackedNodeSpace::total_space()already models the correct layout, consider replacing thefences_fitguard at line 135 with a check againstspace.total_space()instead to eliminate the discrepancy.The condition
included_count > packedat line 142 is unreachable. After each iteration (whether via the recompute path at line 151 or the incremental path at line 160),included_countequalspacked. On the next iteration,packedincrements strictly whileincluded_countis not modified until one of those paths executes again, soincluded_countcan never exceedpacked.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doradb-storage/src/index/btree/algo.rs` around lines 130 - 172, Replace the conservative fences_fit::<V>(lower_fence, upper_fence) guard with a check based on the actual PackedNodeSpace for the fence pair: construct space via PackedNodeSpace::with_fences(lower_fence, upper_fence) (as already done) and rely on space.total_space() together with included_space to determine fit (i.e., compare space.total_space().checked_add(included_space) to BTREE_NODE_USABLE_SIZE) instead of using fences_fit; also remove the unreachable branch condition included_count > packed (and its associated recompute path) since included_count will never be greater than packed given the loop logic (keep the incremental accumulation path that advances included_count to packed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@doradb-storage/src/index/btree/algo.rs`:
- Around line 451-475: The test name misrepresents what it covers: update the
test function test_pack_sibling_node_accounts_for_upper_fence_capacity to either
(A) rename it to reflect the actual behavior (e.g.
test_pack_sibling_node_rejects_when_fences_exceed_page) or (B) change the setup
so the upper-fence path is exercised by adding a second entry (create a second
PackedNodeEntry and adjust entries and assertions) so that pack_sibling_node's
finite-fence loop (the branch that considers the upper fence) is actually
entered; keep references to pack_sibling_node, PackedNodeParams,
lower_fence/rightmost_count/min_slots and assert the expected
Error::InvalidArgument outcome when appropriate.
- Around line 130-172: Replace the conservative fences_fit::<V>(lower_fence,
upper_fence) guard with a check based on the actual PackedNodeSpace for the
fence pair: construct space via PackedNodeSpace::with_fences(lower_fence,
upper_fence) (as already done) and rely on space.total_space() together with
included_space to determine fit (i.e., compare
space.total_space().checked_add(included_space) to BTREE_NODE_USABLE_SIZE)
instead of using fences_fit; also remove the unreachable branch condition
included_count > packed (and its associated recompute path) since included_count
will never be greater than packed given the loop logic (keep the incremental
accumulation path that advances included_count to packed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6cad0a4-4a63-4c24-91fa-ad5891479b2a
📒 Files selected for processing (2)
doradb-storage/src/index/btree/algo.rsdoradb-storage/src/index/btree/node.rs
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doradb-storage/src/index/btree/algo.rs`:
- Around line 471-477: The fences_fit function currently uses the conservative
SpaceEstimation, which rejects keys that would fit once prefix-compression is
applied; change fences_fit to use the prefix-aware estimator (PackedNodeSpace)
so common-prefix compression is considered — replace the
SpaceEstimation::with_fences(...) call with the prefix-aware estimator (e.g.
PackedNodeSpace::with_fences(lower_fence, upper_fence,
V::ENCODED_LEN).total_space() or the project’s prefix-aware estimator API) and
remove any unnecessary ::<V> generic qualifiers at call sites so the
packed/prefix-aware estimator is used consistently for fence-fit checks.
In `@doradb-storage/src/index/btree/mod.rs`:
- Around line 1026-1041: The branch-node partial-merge currently sets
lower_fence_value to r_node.value::<BTreeU64>(count) while pack_node_range_box
copies the slot at index count, causing the first child to be reachable twice;
to fix, when r_node.height() != 0 set KnownFenceNodeParams.lower_fence_value to
BTreeU64::INVALID_VALUE (matching split_node) instead of
r_node.value::<BTreeU64>(count), so the right suffix doesn’t duplicate the first
branch child (alternatively you may change the copied range from
count..r_node.count() to (count + 1)..r_node.count(), but prefer the
INVALID_VALUE approach to align with split_node).
In `@doradb-storage/src/index/disk_tree.rs`:
- Around line 1368-1379: The branch write path currently allocates rewrite block
ids in write_one_branch_block (and write_node_block) and
materialize_rewrite_entries can create child rewrite blocks, but on any
subsequent async write failure those ids remain allocated; modify
materialize_rewrite_entries / materialize_rewrite_entry and
write_one_branch_block to track allocated rewrite block ids (both the parent id
from write_one_*_block and any child ids returned by
materialize_rewrite_entries), and on any Err return roll them back by calling
the block-deallocation routine you already use elsewhere (same pattern as other
write paths), ensuring that when a parent write fails you also deallocate any
materialized children; apply the same pattern to the other occurrences
referenced in the comment (the branch write sites around the blocks at the other
lines).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0af6f8c3-5839-4c24-a866-6f0673bc35cf
📒 Files selected for processing (6)
doradb-storage/src/index/btree/algo.rsdoradb-storage/src/index/btree/mod.rsdoradb-storage/src/index/disk_tree.rsdoradb-storage/src/index/secondary_index.rsdoradb-storage/src/table/tests.rsdoradb-storage/src/trx/recover.rs
✅ Files skipped from review due to trivial changes (2)
- doradb-storage/src/trx/recover.rs
- doradb-storage/src/index/secondary_index.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doradb-storage/src/index/btree/mod.rs`:
- Around line 973-974: The code wrongly uses V::ENCODED_LEN when deleting the
separator from the parent branch (p_node.delete_at(p_r_idx, value_size)) which
uses child page ids encoded with the parent branch width; replace V::ENCODED_LEN
with the parent branch's encoded value length—obtain the length from the parent
node/type (e.g., use the branch value constant or a method on p_node such as
branch/value encoded length) and pass that to p_node.delete_at instead of
V::ENCODED_LEN to avoid corrupting the parent layout.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56746692-5aaf-4622-906d-9236e50ac648
⛔ Files ignored due to path filters (2)
docs/backlogs/000092-refactor-memtree-compaction-policy.mdis excluded by!docs/**,!**/*.mddocs/backlogs/next-idis excluded by!docs/**
📒 Files selected for processing (3)
doradb-storage/src/index/btree/algo.rsdoradb-storage/src/index/btree/mod.rsdoradb-storage/src/index/disk_tree.rs
✅ Files skipped from review due to trivial changes (1)
- doradb-storage/src/index/btree/algo.rs
1 participant
Closes #565
Summary by CodeRabbit