Implementation of KVStore::remove #4
Conversation
| chunk_from_second.offset += first_size; | ||
| }); | ||
|
|
||
| ans.chunks_.back().offset = first_size + second.chunks_.back().offset; |
There was a problem hiding this comment.
Please add regression test for this fix.
| while (items.size() < n) { | ||
| for (usize i = items.size(); i < n; ++i) { | ||
| char ch = '_' + (i & 31); | ||
| items.emplace_back(this->key_generator_(rng, store), |
There was a problem hiding this comment.
We should probably check to make sure we don't generate one of the to_delete keys, since that would bring the key back to life.
src/turtle_kv/kv_store.cpp
Outdated
| *value = combine(*value, *delta_value); | ||
| if (!value->needs_combine()) { | ||
| this->metrics_.delta_log2_get_count[batt::log2_ceil(observed_deltas_size - i)].add(1); | ||
| if (value->is_delete()) { |
There was a problem hiding this comment.
Consider abstracting to remove the duplicate pattern.
| } | ||
| } | ||
|
|
||
| TEST(MergeCompactor, ResultSetConcat) |
There was a problem hiding this comment.
Good test; I have a few suggestions for improvement:
- A few comments, to structure the different phases of the test visually, would be nice
- Have test cases for boundary conditions, like one or the other input to
concatbeing empty - Verify failure for invalid inputs (like overlapping or swapped key ranges are rejected)
- It would be good to test that "fragmented" result sets are property concat'ed: so you could drop some key ranges:
- drop from first and or second
- drop from beginning, middle, or end
- Have a test where the two inputs are differently-sized
| items.emplace_back(delete_key, ValueView::deleted()); | ||
| } | ||
| if (items.size() > n) { | ||
| ResultSet result; |
There was a problem hiding this comment.
Should we maybe truncate items in this case? Or just error?
src/turtle_kv/tree/subtree.hpp
Outdated
| * | ||
| * If no merge, returns None. | ||
| */ | ||
| StatusOr<Optional<Subtree>> try_merge(BatchUpdateContext& context, Subtree& sibling) noexcept; |
There was a problem hiding this comment.
After chatting, new design will be:
siblingis always the right-siblingsiblingpassed as rvalue- modify
thisin place instead of returning a copy on success - unify the merge and borrow cases, by having
try_mergereturnNoneif its a 2 -> 1 merge, otherwise have it return the new right-sibling if its a 2 -> 2 borrow-style merge; that way the convention is the same astry_split.
src/turtle_kv/tree/subtree.hpp
Outdated
| */ | ||
| StatusOr<Optional<Subtree>> try_merge(BatchUpdateContext& context, Subtree& sibling) noexcept; | ||
|
|
||
| /** \brief Attempts to make the Subtree viable by borrowing data from one of its siblings. |
There was a problem hiding this comment.
This can be removed from Subtree after the previous comment is addressed.
src/turtle_kv/tree/subtree.cpp
Outdated
| BATT_ASSIGN_OK_RESULT(std::unique_ptr<InMemoryLeaf> merged_leaf, // | ||
| leaf->try_merge(context, std::move(sibling_leaf_ptr))); | ||
|
|
||
| BATT_CHECK_EQ(merged_leaf, nullptr); |
There was a problem hiding this comment.
This should be handled the same as lines 622-624 below; then we should have a single generic implementation to replace the two lambdas.
src/turtle_kv/tree/subtree.cpp
Outdated
| auto new_leaf = std::make_unique<InMemoryLeaf>(llfs::PinnedPage{}, tree_options); | ||
|
|
||
| new_leaf->result_set = update.result_set; | ||
| new_leaf->result_set = std::move(update.context.decay_batch_to_items(update.result_set)); |
There was a problem hiding this comment.
no std::move is needed here.
src/turtle_kv/tree/subtree.cpp
Outdated
| update.context.merge_compact_edits</*decay_to_items=*/true>( // | ||
| global_max_key(), | ||
| [&](MergeCompactor& compactor) -> Status { | ||
| compactor.push_level(update.result_set.live_edit_slices()); |
There was a problem hiding this comment.
Are we missing a call to update_has_page_refs in this case?
| // Case: {BatchUpdate} + {InMemoryLeaf} => InMemoryLeaf | ||
|
|
||
| BATT_CHECK_EQ(parent_height, 2); | ||
|
|
There was a problem hiding this comment.
We should unify this case (most of it) with the Batch + PackedLeaf => InMemoryLeaf case above (generic impl)
| return OkStatus(); | ||
| }, | ||
| [&](NeedsSplit needs_split) { | ||
| if (needs_split.too_many_segments && !needs_split.too_many_pivots && |
There was a problem hiding this comment.
Would you please add a TODO, to revisit this once the latest VLDB changes are merged in?
src/turtle_kv/tree/subtree.cpp
Outdated
| }, | ||
|
|
||
| [&](const std::unique_ptr<InMemoryLeaf>& leaf [[maybe_unused]]) -> Status { | ||
| return OkStatus(); |
There was a problem hiding this comment.
We should check to see if there are literally no items in the leaf, and if so, replace with an empty tree.
src/turtle_kv/tree/subtree.cpp
Outdated
| this->impl_, | ||
|
|
||
| [&](const llfs::PageIdSlot& page_id_slot [[maybe_unused]]) -> Status { | ||
| return {batt::StatusCode::kUnimplemented}; |
There was a problem hiding this comment.
Maybe return OkStatus if the page id is invalid?
| } else { | ||
| const ParallelAlgoDefaults& algo_defaults = parallel_algo_defaults(); | ||
|
|
||
| const auto src_begin = packed_items.begin(); |
There was a problem hiding this comment.
This code looks good; but it is already available in: batteries/algo/parallel_transform.hpp
|
|
||
| //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - | ||
| // | ||
| StatusOr<std::unique_ptr<InMemoryLeaf>> InMemoryLeaf::try_merge( |
There was a problem hiding this comment.
This looks good for now; please add a TODO to handle the re-balancing case here instead of having re-balance = merge-then-split, as the former is more efficient.
| //==#==========+==+=+=++=+++++++++++-+-+--+----- --- -- - - - - | ||
| // | ||
| Status InMemoryLeaf::apply_batch_update(BatchUpdate& update, | ||
| Optional<BoxedSeq<EditSlice>>&& current_result_set) noexcept |
There was a problem hiding this comment.
I think we should not pass the second argument here; instead, we can make this->result_set_ be Optional, and lazily initialize it in this function.
2 participants
Support for deleting a key/value pair from the KVStore.