From 3becc9409e5a9c7c7ae719da18cb80eef186b6e3 Mon Sep 17 00:00:00 2001 From: Levi Tamasi Date: Mon, 4 Nov 2024 18:06:07 -0800 Subject: [PATCH] Some small improvements around allow_unprepared_value and multi-CF iterators (#13113) Summary: Pull Request resolved: https://github.com/facebook/rocksdb/pull/13113 The patch makes some small improvements related to `allow_unprepared_value` and multi-CF iterators as groundwork for further changes: 1) Similarly to `BaseDeltaIterator`'s base iterator, `MultiCfIteratorImpl` gets passed its child iterators by the client. Even though they are currently guaranteed to have been created using the same read options as each other and the multi-CF iterator, it is safer to not assume this and call `PrepareValue` unconditionally before using any child iterator's `value()` or `columns()`. 2) Again similarly to `BaseDeltaIterator`, it makes sense to pass the entire `ReadOptions` structure to `MultiCfIteratorImpl` in case it turns out to require other read options in the future. 3) The constructors of the various multi-CF iterator classes now take an rvalue reference to a vector of column family handle + `unique_ptr` to child iterator pairs and use move semantics to take ownership of this vector (instead of taking two separate vectors of column family handles and raw iterator pointers). 4) Constructor arguments and the members of `MultiCfIteratorImpl` are reordered for consistency. Reviewed By: jowlyzhang Differential Revision: D65407521 fbshipit-source-id: 66c2c689ec8b036740bd98641b7b5c0ff7e777f2 --- db/attribute_group_iterator_impl.h | 10 +-- db/coalescing_iterator.h | 11 +-- db/db_impl/db_impl.cc | 18 ++++- db/multi_cf_iterator_impl.h | 106 +++++++++++++---------------- 4 files changed, 73 insertions(+), 72 deletions(-) diff --git a/db/attribute_group_iterator_impl.h b/db/attribute_group_iterator_impl.h index c6dc2722a3..d3318e79ea 100644 --- a/db/attribute_group_iterator_impl.h +++ b/db/attribute_group_iterator_impl.h @@ -13,11 +13,11 @@ namespace ROCKSDB_NAMESPACE { class AttributeGroupIteratorImpl : public AttributeGroupIterator { public: AttributeGroupIteratorImpl( - const Comparator* comparator, bool allow_unprepared_value, - const std::vector& column_families, - const std::vector& child_iterators) - : impl_(comparator, allow_unprepared_value, column_families, - child_iterators, ResetFunc(this), PopulateFunc(this)) {} + const ReadOptions& read_options, const Comparator* comparator, + std::vector>>&& + cfh_iter_pairs) + : impl_(read_options, comparator, std::move(cfh_iter_pairs), + ResetFunc(this), PopulateFunc(this)) {} ~AttributeGroupIteratorImpl() override {} // No copy allowed diff --git a/db/coalescing_iterator.h b/db/coalescing_iterator.h index c4a1c831ed..ae00f004ca 100644 --- a/db/coalescing_iterator.h +++ b/db/coalescing_iterator.h @@ -12,11 +12,12 @@ namespace ROCKSDB_NAMESPACE { // EXPERIMENTAL class CoalescingIterator : public Iterator { public: - CoalescingIterator(const Comparator* comparator, bool allow_unprepared_value, - const std::vector& column_families, - const std::vector& child_iterators) - : impl_(comparator, allow_unprepared_value, column_families, - child_iterators, ResetFunc(this), PopulateFunc(this)) {} + CoalescingIterator( + const ReadOptions& read_options, const Comparator* comparator, + std::vector>>&& + cfh_iter_pairs) + : impl_(read_options, comparator, std::move(cfh_iter_pairs), + ResetFunc(this), PopulateFunc(this)) {} ~CoalescingIterator() override {} // No copy allowed diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index 3fabfabe31..d9cd321f37 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -3998,14 +3998,26 @@ std::unique_ptr DBImpl::NewMultiCfIterator( "Different comparators are being used across CFs")); } } + std::vector child_iterators; Status s = NewIterators(_read_options, column_families, &child_iterators); if (!s.ok()) { return error_iterator_func(s); } - return std::make_unique( - column_families[0]->GetComparator(), _read_options.allow_unprepared_value, - column_families, std::move(child_iterators)); + + assert(column_families.size() == child_iterators.size()); + + std::vector>> + cfh_iter_pairs; + cfh_iter_pairs.reserve(column_families.size()); + for (size_t i = 0; i < column_families.size(); ++i) { + cfh_iter_pairs.emplace_back(column_families[i], + std::unique_ptr(child_iterators[i])); + } + + return std::make_unique(_read_options, + column_families[0]->GetComparator(), + std::move(cfh_iter_pairs)); } Status DBImpl::NewIterators( diff --git a/db/multi_cf_iterator_impl.h b/db/multi_cf_iterator_impl.h index 4bea05a5be..67a59dddf9 100644 --- a/db/multi_cf_iterator_impl.h +++ b/db/multi_cf_iterator_impl.h @@ -24,24 +24,18 @@ struct MultiCfIteratorInfo { template class MultiCfIteratorImpl { public: - MultiCfIteratorImpl(const Comparator* comparator, bool allow_unprepared_value, - const std::vector& column_families, - const std::vector& child_iterators, - ResetFunc reset_func, PopulateFunc populate_func) - : comparator_(comparator), - allow_unprepared_value_(allow_unprepared_value), - heap_(MultiCfMinHeap( - MultiCfHeapItemComparator>(comparator_))), + MultiCfIteratorImpl( + const ReadOptions& read_options, const Comparator* comparator, + std::vector>>&& + cfh_iter_pairs, + ResetFunc reset_func, PopulateFunc populate_func) + : allow_unprepared_value_(read_options.allow_unprepared_value), + comparator_(comparator), + cfh_iter_pairs_(std::move(cfh_iter_pairs)), reset_func_(std::move(reset_func)), - populate_func_(std::move(populate_func)) { - assert(column_families.size() > 0 && - column_families.size() == child_iterators.size()); - cfh_iter_pairs_.reserve(column_families.size()); - for (size_t i = 0; i < column_families.size(); ++i) { - cfh_iter_pairs_.emplace_back( - column_families[i], std::unique_ptr(child_iterators[i])); - } - } + populate_func_(std::move(populate_func)), + heap_(MultiCfMinHeap( + MultiCfHeapItemComparator>(comparator_))) {} ~MultiCfIteratorImpl() { status_.PermitUncheckedError(); } // No copy allowed @@ -108,38 +102,21 @@ class MultiCfIteratorImpl { return true; } - auto prepare_value_func = [this](auto& heap, Iterator* iterator) { - assert(iterator); - assert(iterator->Valid()); - assert(iterator->status().ok()); - - if (!iterator->PrepareValue()) { - assert(!iterator->Valid()); - assert(!iterator->status().ok()); - - considerStatus(iterator->status()); - assert(!status_.ok()); - - heap.clear(); - return false; - } - - return true; - }; - if (std::holds_alternative(heap_)) { - return PopulateIterator(std::get(heap_), - prepare_value_func); + return PopulateIterator(std::get(heap_)); } - return PopulateIterator(std::get(heap_), - prepare_value_func); + return PopulateIterator(std::get(heap_)); } private: + Status status_; + bool allow_unprepared_value_; + const Comparator* comparator_; std::vector>> cfh_iter_pairs_; - Status status_; + ResetFunc reset_func_; + PopulateFunc populate_func_; template class MultiCfHeapItemComparator { @@ -161,9 +138,6 @@ class MultiCfIteratorImpl { const Comparator* comparator_; }; - const Comparator* comparator_; - bool allow_unprepared_value_; - using MultiCfMinHeap = BinaryHeap>>; @@ -174,9 +148,6 @@ class MultiCfIteratorImpl { MultiCfIterHeap heap_; - ResetFunc reset_func_; - PopulateFunc populate_func_; - Iterator* current() const { if (std::holds_alternative(heap_)) { auto& max_heap = std::get(heap_); @@ -230,10 +201,8 @@ class MultiCfIteratorImpl { ++i; } if (!allow_unprepared_value_ && !heap.empty()) { - [[maybe_unused]] const bool result = PopulateIterator( - heap, - [](auto& /* heap */, Iterator* /* iterator */) { return true; }); - assert(result); + [[maybe_unused]] const bool result = PopulateIterator(heap); + assert(result || (!Valid() && !status_.ok())); } } @@ -300,15 +269,13 @@ class MultiCfIteratorImpl { } if (!allow_unprepared_value_ && !heap.empty()) { - [[maybe_unused]] const bool result = PopulateIterator( - heap, - [](auto& /* heap */, Iterator* /* iterator */) { return true; }); - assert(result); + [[maybe_unused]] const bool result = PopulateIterator(heap); + assert(result || (!Valid() && !status_.ok())); } } - template - bool PopulateIterator(BinaryHeap& heap, PrepareValueFunc prepare_value_func) { + template + bool PopulateIterator(BinaryHeap& heap) { // 1. Keep the top iterator (by popping it from the heap) and add it to list // to populate // 2. For all non-top iterators having the same key as top iter popped @@ -319,12 +286,33 @@ class MultiCfIteratorImpl { // collected in step 1 and 2 and add all the iters back to the heap assert(!heap.empty()); + auto prepare_value = [this, &heap](Iterator* iterator) { + assert(iterator); + assert(iterator->Valid()); + assert(iterator->status().ok()); + + if (!iterator->PrepareValue()) { + assert(!iterator->Valid()); + assert(!iterator->status().ok()); + + considerStatus(iterator->status()); + heap.clear(); + + assert(!Valid()); + assert(!status_.ok()); + + return false; + } + + return true; + }; + auto top = heap.top(); assert(top.iterator); assert(top.iterator->Valid()); assert(top.iterator->status().ok()); - if (!prepare_value_func(heap, top.iterator)) { + if (!prepare_value(top.iterator)) { return false; } @@ -344,7 +332,7 @@ class MultiCfIteratorImpl { break; } - if (!prepare_value_func(heap, current.iterator)) { + if (!prepare_value(current.iterator)) { return false; }