KVStore::remove Implementation Part 2 by vidyasilai · Pull Request #4 · mathworks/turtle_kv

vidyasilai · 2025-11-12T16:29:38Z

This PR contains the tree algorithm code related to deletions:

Node and leaf merging
New InMemoryNode update buffer level type HybridLevel

src/turtle_kv/core/testing/generate.hpp

src/turtle_kv/kv_store_scanner.cpp

src/turtle_kv/core/merge_compactor.cpp

src/turtle_kv/core/testing/generate.hpp

src/turtle_kv/kv_store.cpp

src/turtle_kv/kv_store_scanner.cpp

src/turtle_kv/core/merge_compactor.test.cpp

src/turtle_kv/core/testing/generate.hpp

src/turtle_kv/tree/subtree.hpp

src/turtle_kv/tree/subtree.cpp

tonyastolfi · 2026-03-31T15:17:57Z

src/turtle_kv/tree/subtree.cpp

+
+  // If the root is a leaf and there are no items in the leaf, set the root to be an empty subtree.
+  //
+  if (batt::is_case<std::unique_ptr<InMemoryLeaf>>(this->impl_)) {


Looks right, but maybe it should be in try_shrink instead..?

tonyastolfi · 2026-03-31T15:19:20Z

src/turtle_kv/tree/subtree.cpp

+//
+Status Subtree::try_merge(BatchUpdateContext& context, Subtree&& sibling) noexcept
+{
+  BATT_CHECK(!this->locked_.load());


Maybe assert/check that this height and sibling height are equal?

tonyastolfi · 2026-03-31T15:26:48Z

src/turtle_kv/tree/in_memory_node.cpp

+  //
+  i32 sibling_i = pivot_i;
+
+  if (pivot_i == 0) {


Should just be 2 cases.

tonyastolfi · 2026-03-31T15:28:29Z

src/turtle_kv/tree/subtree.cpp

+      [&](auto& in_memory) -> Status {
+        using PtrT = std::decay_t<decltype(in_memory)>;
+
+        BATT_CHECK(batt::is_case<PtrT>(sibling.impl_));


Would be good to add a message that interprets what this panic means.

tonyastolfi · 2026-03-31T15:28:52Z

src/turtle_kv/tree/subtree.cpp

+
+        BATT_CHECK(batt::is_case<PtrT>(sibling.impl_));
+        auto& sibling_ptr = std::get<PtrT>(sibling.impl_);
+        BATT_CHECK(sibling_ptr);


nit: BATT_CHECK_NOT_NULLPTR

tonyastolfi · 2026-03-31T15:30:38Z

src/turtle_kv/tree/in_memory_node.cpp

+                         update_context.page_loader,
+                         update_context.overcommit)
+          .merge_pivots(left_pivot_i, right_pivot_i);
+    }


Maybe comment why we don't care about the other cases?

Should probably be a batt::case_of

tonyastolfi · 2026-03-31T15:32:11Z

src/turtle_kv/tree/in_memory_node.cpp

+  this->pending_bytes[left_pivot_i] += this->pending_bytes[right_pivot_i];
+  this->pending_bytes.erase(this->pending_bytes.begin() + right_pivot_i);
+
+  bool is_pending_bytes_exact = get_bit(this->pending_bytes_is_exact, left_pivot_i) &


& -> &&

tonyastolfi · 2026-03-31T15:51:57Z

src/turtle_kv/tree/in_memory_leaf.cpp

+  BATT_CHECK(sibling->result_set);
+
+  if (sibling->result_set->empty()) {
+    BATT_CHECK(batt::is_case<Viable>(this->get_viability()));


Message would be helpful here.

tonyastolfi · 2026-03-31T15:57:51Z

src/turtle_kv/tree/in_memory_leaf.cpp

+    this->edit_size_totals = std::move(sibling->edit_size_totals);
+    return OkStatus();
+  }
+


We should probably retain a pin on the sibling's leaf page in this; that means we may need to change InMemoryLeaf::pinned_leaf_page_ into pinned_leaf_pages_.

In regard to turning pinned_leaf_page_ into a vector of pinned pages, what are your thoughts on keeping pinned_leaf_page_ as is and adding another member variable to InMemoryLeaf that is of type std::vector<PinnedPage> that holds pins on sibling pages as a result of merges? My reason for suggesting this is due to the logic in InMemoryLeaf::apply_batch_update, where we check the state of pinned_leaf_page_ to determine how to extract result_set.

tonyastolfi · 2026-04-07T15:49:08Z

src/turtle_kv/tree/subtree.cpp

+    if (flush_status == batt::StatusCode::kUnavailable) {
+      BATT_REQUIRE_OK(this->try_shrink());
+    }
+


After a successful flush, we should do something here to see if the root object has changed; if it has, then we should reset the retries counter, since limiting to kMaxPivots is based on the assumption of a single node.

tonyastolfi · 2026-04-07T16:02:40Z

src/turtle_kv/tree/subtree_viability.hpp

+        return false;
+      },
+      [](const NeedsMerge& needs_merge) {
+        return !needs_merge.single_pivot;


&& !needs_merge.no_items

tonyastolfi · 2026-04-07T16:05:00Z

src/turtle_kv/tree/packed_leaf_page.hpp

@@ -441,6 +441,9 @@ inline usize PackedLeafLayoutPlan::compute_trie_step_size() const
    step_size = 1;
  } else if (this->key_count < 256) {
    step_size = this->key_count / 16;


Consider rounding up instead..?

tonyastolfi · 2026-04-07T16:15:32Z

src/turtle_kv/tree/active_pivots_set.hpp

+
+  /** \brief Removes the specified number (`count`) pivots from the end of the bit set.
+   */
+  BATT_ALWAYS_INLINE void pop_back_pivots(i32 count)


maybe push_front_pivots?

tonyastolfi · 2026-04-07T16:23:42Z

src/turtle_kv/tree/in_memory_node.cpp

+
+        // Erase potential duplicate segments that have resulted from a previous split.
+        //
+        this->segments.erase(std::unique(this->segments.begin(),


We do need to do this de-duplication step, but we need to calculate:

union of the active pivots set

"joining" of the portions of the piecewise-filter that correspond to each sibling's "side."

tonyastolfi · 2026-04-08T14:49:04Z

src/turtle_kv/tree/algo/segments.hpp

+  {
+    bool left_is_active = this->segment_.is_pivot_active(left_pivot);
+    bool right_is_active = this->segment_.is_pivot_active(right_pivot);
+    this->segment_.set_pivot_active(left_pivot, left_is_active | right_is_active);


| -> ||

vidyasilai added 3 commits November 11, 2025 10:57

Initial commit

47a4572

Added some more comments

47dc604

Remove conan file changes

99d750f

vidyasilai self-assigned this Nov 12, 2025

vidyasilai requested a review from tonyastolfi November 12, 2025 16:29

vidyasilai added 4 commits November 15, 2025 12:38

Some bug fixes; new test case

969c140

More bug fixes

ca5b89b

KVStoreScanner updates

51a4e02

Subtree test config change

b126c03

vidyasilai marked this pull request as ready for review November 18, 2025 15:27

tonyastolfi reviewed Nov 18, 2025

View reviewed changes

src/turtle_kv/core/testing/generate.hpp Outdated Show resolved Hide resolved

tonyastolfi reviewed Nov 18, 2025

View reviewed changes

src/turtle_kv/kv_store_scanner.cpp Show resolved Hide resolved

tonyastolfi reviewed Nov 18, 2025

View reviewed changes

src/turtle_kv/kv_store_scanner.cpp Outdated Show resolved Hide resolved

vidyasilai added 2 commits November 21, 2025 09:48

First round of feedback

afa9940

Improvements to Subtree test

8dbab04

tonyastolfi reviewed Nov 25, 2025

View reviewed changes

src/turtle_kv/core/merge_compactor.cpp Show resolved Hide resolved

tonyastolfi reviewed Nov 25, 2025

View reviewed changes

src/turtle_kv/core/testing/generate.hpp Outdated Show resolved Hide resolved

tonyastolfi reviewed Nov 25, 2025

View reviewed changes

src/turtle_kv/kv_store.cpp Outdated Show resolved Hide resolved

tonyastolfi reviewed Nov 25, 2025

View reviewed changes

src/turtle_kv/kv_store_scanner.cpp Show resolved Hide resolved

vidyasilai added 2 commits November 28, 2025 11:48

More feedback

969efc0

Small style updates

84112d8

tonyastolfi reviewed Dec 3, 2025

View reviewed changes

src/turtle_kv/core/merge_compactor.test.cpp Outdated Show resolved Hide resolved

tonyastolfi reviewed Dec 3, 2025

View reviewed changes

src/turtle_kv/core/testing/generate.hpp Outdated Show resolved Hide resolved

tonyastolfi reviewed Dec 3, 2025

View reviewed changes

src/turtle_kv/tree/subtree.hpp Outdated Show resolved Hide resolved

tonyastolfi reviewed Dec 3, 2025

View reviewed changes

src/turtle_kv/tree/subtree.hpp Outdated Show resolved Hide resolved

More feedback, fix try_merge interface

75e815a

tonyastolfi reviewed Dec 9, 2025

View reviewed changes

src/turtle_kv/tree/subtree.cpp Outdated Show resolved Hide resolved

tonyastolfi reviewed Dec 9, 2025

View reviewed changes

src/turtle_kv/tree/subtree.cpp Outdated Show resolved Hide resolved

tonyastolfi reviewed Dec 9, 2025

View reviewed changes

src/turtle_kv/tree/subtree.cpp Outdated Show resolved Hide resolved

tonyastolfi reviewed Dec 9, 2025

View reviewed changes

src/turtle_kv/tree/subtree.cpp Show resolved Hide resolved

tonyastolfi and others added 9 commits March 16, 2026 15:35

assert size of packed active pivots set.

6e01a50

Static asserts: ActivePivotsSet.

dde7018

Revert change.

1c7efe1

Start adding HybridLevel

d0f1745

Test changes

fa6a609

Finish adding HybridLevel

b3d7ec2

Merge with main

c7714d9

Bug fixes

eb53e2f

Fix some comments

43e38f2

tonyastolfi reviewed Mar 24, 2026

View reviewed changes

src/turtle_kv/tree/subtree.cpp Outdated Show resolved Hide resolved

vidyasilai added 2 commits March 31, 2026 10:27

Fix Subtree::flush_and_shrink loop logic

dd18c3d

Small style fix

f92f249

tonyastolfi reviewed Mar 31, 2026

View reviewed changes

src/turtle_kv/tree/subtree.cpp Outdated Show resolved Hide resolved

tonyastolfi reviewed Mar 31, 2026

View reviewed changes

vidyasilai added 2 commits March 31, 2026 15:30

Some feedback changes

d2621d7

Some more Subtree updates

0def570

tonyastolfi reviewed Apr 7, 2026

View reviewed changes

tonyastolfi reviewed Apr 8, 2026

View reviewed changes

Conversation

vidyasilai commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonyastolfi Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonyastolfi Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

vidyasilai commented Nov 12, 2025 •

edited

Loading

tonyastolfi Mar 31, 2026 •

edited

Loading

tonyastolfi Apr 7, 2026 •

edited

Loading