From 53e595d1f30383a77e08bf8f388c5a7a4e3220c3 Mon Sep 17 00:00:00 2001 From: mrambacher Date: Wed, 6 Oct 2021 07:47:28 -0700 Subject: [PATCH] Cleanup multiple implementations of VectorIterator (#8901) Summary: There were three implementations of VectorIterator (util/vector_iterator, test_util/testutil.h and LoggingForwardVectorIterator). Merged them into one class to increase code coverage/testing and reduce duplication. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8901 Reviewed By: pdillinger Differential Revision: D31022673 Pulled By: mrambacher fbshipit-source-id: 8e3acbd2dfd60b4df609d02cc72846de2389d531 --- db/blob/blob_counting_iterator_test.cc | 5 ++- db/compaction/clipping_iterator_test.cc | 8 ++-- db/compaction/compaction_iterator_test.cc | 32 ++++++-------- db/merge_helper_test.cc | 14 +++--- db/range_del_aggregator_bench.cc | 9 ++-- db/range_del_aggregator_test.cc | 5 ++- db/range_tombstone_fragmenter_test.cc | 6 ++- table/merger_test.cc | 5 ++- test_util/testutil.h | 52 ----------------------- util/vector_iterator.h | 46 +++++++++++++------- 10 files changed, 74 insertions(+), 108 deletions(-) diff --git a/db/blob/blob_counting_iterator_test.cc b/db/blob/blob_counting_iterator_test.cc index 12ccbc75a..faa329e81 100644 --- a/db/blob/blob_counting_iterator_test.cc +++ b/db/blob/blob_counting_iterator_test.cc @@ -14,6 +14,7 @@ #include "db/dbformat.h" #include "test_util/testharness.h" #include "test_util/testutil.h" +#include "util/vector_iterator.h" namespace ROCKSDB_NAMESPACE { @@ -67,7 +68,7 @@ TEST(BlobCountingIteratorTest, CountBlobs) { assert(keys.size() == values.size()); - test::VectorIterator input(keys, values); + VectorIterator input(keys, values); BlobGarbageMeter blob_garbage_meter; BlobCountingIterator blob_counter(&input, &blob_garbage_meter); @@ -307,7 +308,7 @@ TEST(BlobCountingIteratorTest, CorruptBlobIndex) { assert(keys.size() == values.size()); - test::VectorIterator input(keys, values); + VectorIterator input(keys, values); BlobGarbageMeter blob_garbage_meter; BlobCountingIterator blob_counter(&input, &blob_garbage_meter); diff --git a/db/compaction/clipping_iterator_test.cc b/db/compaction/clipping_iterator_test.cc index 3a31b61eb..a9c2eb356 100644 --- a/db/compaction/clipping_iterator_test.cc +++ b/db/compaction/clipping_iterator_test.cc @@ -10,22 +10,24 @@ #include #include +#include "db/dbformat.h" #include "rocksdb/comparator.h" #include "test_util/testharness.h" #include "test_util/testutil.h" +#include "util/vector_iterator.h" namespace ROCKSDB_NAMESPACE { // A vector iterator which does its own bounds checking. This is for testing the // optimizations in the clipping iterator where we bypass the bounds checking if // the input iterator has already performed it. -class BoundsCheckingVectorIterator : public test::VectorIterator { +class BoundsCheckingVectorIterator : public VectorIterator { public: BoundsCheckingVectorIterator(const std::vector& keys, const std::vector& values, const Slice* start, const Slice* end, const Comparator* cmp) - : VectorIterator(keys, values), start_(start), end_(end), cmp_(cmp) { + : VectorIterator(keys, values, cmp), start_(start), end_(end), cmp_(cmp) { assert(cmp_); } @@ -105,7 +107,7 @@ TEST_P(ClippingIteratorTest, Clip) { use_bounds_checking_vec_it ? new BoundsCheckingVectorIterator(input_keys, input_values, &start, &end, BytewiseComparator()) - : new test::VectorIterator(input_keys, input_values)); + : new VectorIterator(input_keys, input_values, BytewiseComparator())); ClippingIterator clip(input.get(), &start, &end, BytewiseComparator()); diff --git a/db/compaction/compaction_iterator_test.cc b/db/compaction/compaction_iterator_test.cc index 6013ec69a..a699b840d 100644 --- a/db/compaction/compaction_iterator_test.cc +++ b/db/compaction/compaction_iterator_test.cc @@ -3,15 +3,17 @@ // COPYING file in the root directory) and Apache 2.0 License // (found in the LICENSE.Apache file in the root directory). +#include "db/compaction/compaction_iterator.h" #include #include -#include "db/compaction/compaction_iterator.h" +#include "db/dbformat.h" #include "port/port.h" #include "test_util/testharness.h" #include "test_util/testutil.h" #include "util/string_util.h" +#include "util/vector_iterator.h" #include "utilities/merge_operators.h" namespace ROCKSDB_NAMESPACE { @@ -86,7 +88,7 @@ class FilterAllKeysCompactionFilter : public CompactionFilter { const char* Name() const override { return "AllKeysCompactionFilter"; } }; -class LoggingForwardVectorIterator : public InternalIterator { +class LoggingForwardVectorIterator : public VectorIterator { public: struct Action { enum class Type { @@ -108,22 +110,19 @@ class LoggingForwardVectorIterator : public InternalIterator { LoggingForwardVectorIterator(const std::vector& keys, const std::vector& values) - : keys_(keys), values_(values), current_(keys.size()) { - assert(keys_.size() == values_.size()); + : VectorIterator(keys, values) { + current_ = keys_.size(); } - bool Valid() const override { return current_ < keys_.size(); } - void SeekToFirst() override { log.emplace_back(Action::Type::SEEK_TO_FIRST); - current_ = 0; + VectorIterator::SeekToFirst(); } void SeekToLast() override { assert(false); } void Seek(const Slice& target) override { log.emplace_back(Action::Type::SEEK, target.ToString()); - current_ = std::lower_bound(keys_.begin(), keys_.end(), target.ToString()) - - keys_.begin(); + VectorIterator::Seek(target); } void SeekForPrev(const Slice& /*target*/) override { assert(false); } @@ -131,27 +130,20 @@ class LoggingForwardVectorIterator : public InternalIterator { void Next() override { assert(Valid()); log.emplace_back(Action::Type::NEXT); - current_++; + VectorIterator::Next(); } void Prev() override { assert(false); } Slice key() const override { assert(Valid()); - return Slice(keys_[current_]); + return VectorIterator::key(); } Slice value() const override { assert(Valid()); - return Slice(values_[current_]); + return VectorIterator::value(); } - Status status() const override { return Status::OK(); } - std::vector log; - - private: - std::vector keys_; - std::vector values_; - size_t current_; }; class FakeCompaction : public CompactionIterator::CompactionProxy { @@ -244,7 +236,7 @@ class CompactionIteratorTest : public testing::TestWithParam { bool key_not_exists_beyond_output_level = false, const std::string* full_history_ts_low = nullptr) { std::unique_ptr unfragmented_range_del_iter( - new test::VectorIterator(range_del_ks, range_del_vs)); + new VectorIterator(range_del_ks, range_del_vs, &icmp_)); auto tombstone_list = std::make_shared( std::move(unfragmented_range_del_iter), icmp_); std::unique_ptr range_del_iter( diff --git a/db/merge_helper_test.cc b/db/merge_helper_test.cc index 117916c8f..597eb5931 100644 --- a/db/merge_helper_test.cc +++ b/db/merge_helper_test.cc @@ -3,30 +3,33 @@ // COPYING file in the root directory) and Apache 2.0 License // (found in the LICENSE.Apache file in the root directory). +#include "db/merge_helper.h" + #include #include #include -#include "db/merge_helper.h" +#include "db/dbformat.h" #include "rocksdb/comparator.h" #include "test_util/testharness.h" #include "test_util/testutil.h" #include "util/coding.h" +#include "util/vector_iterator.h" #include "utilities/merge_operators.h" namespace ROCKSDB_NAMESPACE { class MergeHelperTest : public testing::Test { public: - MergeHelperTest() { env_ = Env::Default(); } + MergeHelperTest() : icmp_(BytewiseComparator()) { env_ = Env::Default(); } ~MergeHelperTest() override = default; Status Run(SequenceNumber stop_before, bool at_bottom, SequenceNumber latest_snapshot = 0) { - iter_.reset(new test::VectorIterator(ks_, vs_)); + iter_.reset(new VectorIterator(ks_, vs_, &icmp_)); iter_->SeekToFirst(); - merge_helper_.reset(new MergeHelper(env_, BytewiseComparator(), + merge_helper_.reset(new MergeHelper(env_, icmp_.user_comparator(), merge_op_.get(), filter_.get(), nullptr, false, latest_snapshot)); return merge_helper_->MergeUntil(iter_.get(), nullptr /* range_del_agg */, @@ -45,7 +48,8 @@ class MergeHelperTest : public testing::Test { } Env* env_; - std::unique_ptr iter_; + InternalKeyComparator icmp_; + std::unique_ptr iter_; std::shared_ptr merge_op_; std::unique_ptr merge_helper_; std::vector ks_; diff --git a/db/range_del_aggregator_bench.cc b/db/range_del_aggregator_bench.cc index 061232f99..651999bd8 100644 --- a/db/range_del_aggregator_bench.cc +++ b/db/range_del_aggregator_bench.cc @@ -19,15 +19,16 @@ int main() { #include #include +#include "db/dbformat.h" #include "db/range_del_aggregator.h" #include "db/range_tombstone_fragmenter.h" #include "rocksdb/comparator.h" #include "rocksdb/system_clock.h" -#include "test_util/testutil.h" #include "util/coding.h" #include "util/gflags_compat.h" #include "util/random.h" #include "util/stop_watch.h" +#include "util/vector_iterator.h" using GFLAGS_NAMESPACE::ParseCommandLineFlags; @@ -146,8 +147,8 @@ std::unique_ptr MakeRangeDelIterator( keys.push_back(key_and_value.first.Encode().ToString()); values.push_back(key_and_value.second.ToString()); } - return std::unique_ptr( - new test::VectorIterator(keys, values)); + return std::unique_ptr( + new VectorIterator(keys, values, &icmp)); } // convert long to a big-endian slice key @@ -207,8 +208,6 @@ int main(int argc, char** argv) { ROCKSDB_NAMESPACE::Key(start), ROCKSDB_NAMESPACE::Key(end), j); } - auto range_del_iter = - ROCKSDB_NAMESPACE::MakeRangeDelIterator(persistent_range_tombstones); fragmented_range_tombstone_lists.emplace_back( new ROCKSDB_NAMESPACE::FragmentedRangeTombstoneList( ROCKSDB_NAMESPACE::MakeRangeDelIterator( diff --git a/db/range_del_aggregator_test.cc b/db/range_del_aggregator_test.cc index 0b8b5079c..c1d739753 100644 --- a/db/range_del_aggregator_test.cc +++ b/db/range_del_aggregator_test.cc @@ -13,6 +13,7 @@ #include "db/dbformat.h" #include "db/range_tombstone_fragmenter.h" #include "test_util/testutil.h" +#include "util/vector_iterator.h" namespace ROCKSDB_NAMESPACE { @@ -30,8 +31,8 @@ std::unique_ptr MakeRangeDelIter( keys.push_back(key_and_value.first.Encode().ToString()); values.push_back(key_and_value.second.ToString()); } - return std::unique_ptr( - new test::VectorIterator(keys, values)); + return std::unique_ptr( + new VectorIterator(keys, values, &bytewise_icmp)); } std::vector> diff --git a/db/range_tombstone_fragmenter_test.cc b/db/range_tombstone_fragmenter_test.cc index 56234b1dd..368dc909f 100644 --- a/db/range_tombstone_fragmenter_test.cc +++ b/db/range_tombstone_fragmenter_test.cc @@ -6,8 +6,10 @@ #include "db/range_tombstone_fragmenter.h" #include "db/db_test_util.h" +#include "db/dbformat.h" #include "rocksdb/comparator.h" #include "test_util/testutil.h" +#include "util/vector_iterator.h" namespace ROCKSDB_NAMESPACE { @@ -25,8 +27,8 @@ std::unique_ptr MakeRangeDelIter( keys.push_back(key_and_value.first.Encode().ToString()); values.push_back(key_and_value.second.ToString()); } - return std::unique_ptr( - new test::VectorIterator(keys, values)); + return std::unique_ptr( + new VectorIterator(keys, values, &bytewise_icmp)); } void CheckIterPosition(const RangeTombstone& tombstone, diff --git a/table/merger_test.cc b/table/merger_test.cc index 13f225731..02eadf0ab 100644 --- a/table/merger_test.cc +++ b/table/merger_test.cc @@ -10,6 +10,7 @@ #include "test_util/testharness.h" #include "test_util/testutil.h" #include "util/random.h" +#include "util/vector_iterator.h" namespace ROCKSDB_NAMESPACE { @@ -101,14 +102,14 @@ class MergerTest : public testing::Test { std::vector small_iterators; for (size_t i = 0; i < num_iterators; ++i) { auto strings = GenerateStrings(strings_per_iterator, letters_per_string); - small_iterators.push_back(new test::VectorIterator(strings)); + small_iterators.push_back(new VectorIterator(strings, strings, &icomp_)); all_keys_.insert(all_keys_.end(), strings.begin(), strings.end()); } merging_iterator_.reset( NewMergingIterator(&icomp_, &small_iterators[0], static_cast(small_iterators.size()))); - single_iterator_.reset(new test::VectorIterator(all_keys_)); + single_iterator_.reset(new VectorIterator(all_keys_, all_keys_, &icomp_)); } InternalKeyComparator icomp_; diff --git a/test_util/testutil.h b/test_util/testutil.h index 0c358b254..b0424947a 100644 --- a/test_util/testutil.h +++ b/test_util/testutil.h @@ -128,58 +128,6 @@ class SimpleSuffixReverseComparator : public Comparator { // endian machines. extern const Comparator* Uint64Comparator(); -// Iterator over a vector of keys/values -class VectorIterator : public InternalIterator { - public: - explicit VectorIterator(const std::vector& keys) - : keys_(keys), current_(keys.size()) { - std::sort(keys_.begin(), keys_.end()); - values_.resize(keys.size()); - } - - VectorIterator(const std::vector& keys, - const std::vector& values) - : keys_(keys), values_(values), current_(keys.size()) { - assert(keys_.size() == values_.size()); - } - - virtual bool Valid() const override { return current_ < keys_.size(); } - - virtual void SeekToFirst() override { current_ = 0; } - virtual void SeekToLast() override { current_ = keys_.size() - 1; } - - virtual void Seek(const Slice& target) override { - current_ = std::lower_bound(keys_.begin(), keys_.end(), target.ToString()) - - keys_.begin(); - } - - virtual void SeekForPrev(const Slice& target) override { - current_ = std::upper_bound(keys_.begin(), keys_.end(), target.ToString()) - - keys_.begin(); - if (!Valid()) { - SeekToLast(); - } else { - Prev(); - } - } - - virtual void Next() override { current_++; } - virtual void Prev() override { current_--; } - - virtual Slice key() const override { return Slice(keys_[current_]); } - virtual Slice value() const override { return Slice(values_[current_]); } - - virtual Status status() const override { return Status::OK(); } - - virtual bool IsKeyPinned() const override { return true; } - virtual bool IsValuePinned() const override { return true; } - - private: - std::vector keys_; - std::vector values_; - size_t current_; -}; - class StringSink : public FSWritableFile { public: std::string contents_; diff --git a/util/vector_iterator.h b/util/vector_iterator.h index fc26ec0c0..b8c3e5e0f 100644 --- a/util/vector_iterator.h +++ b/util/vector_iterator.h @@ -16,18 +16,20 @@ namespace ROCKSDB_NAMESPACE { class VectorIterator : public InternalIterator { public: VectorIterator(std::vector keys, std::vector values, - const InternalKeyComparator* icmp) + const Comparator* icmp = nullptr) : keys_(std::move(keys)), values_(std::move(values)), - indexed_cmp_(icmp, &keys_), - current_(0) { + current_(keys_.size()), + indexed_cmp_(icmp, &keys_) { assert(keys_.size() == values_.size()); indices_.reserve(keys_.size()); for (size_t i = 0; i < keys_.size(); i++) { indices_.push_back(i); } - std::sort(indices_.begin(), indices_.end(), indexed_cmp_); + if (icmp != nullptr) { + std::sort(indices_.begin(), indices_.end(), indexed_cmp_); + } } virtual bool Valid() const override { @@ -38,15 +40,27 @@ class VectorIterator : public InternalIterator { virtual void SeekToLast() override { current_ = indices_.size() - 1; } virtual void Seek(const Slice& target) override { - current_ = std::lower_bound(indices_.begin(), indices_.end(), target, - indexed_cmp_) - - indices_.begin(); + if (indexed_cmp_.cmp != nullptr) { + current_ = std::lower_bound(indices_.begin(), indices_.end(), target, + indexed_cmp_) - + indices_.begin(); + } else { + current_ = + std::lower_bound(keys_.begin(), keys_.end(), target.ToString()) - + keys_.begin(); + } } virtual void SeekForPrev(const Slice& target) override { - current_ = std::lower_bound(indices_.begin(), indices_.end(), target, - indexed_cmp_) - - indices_.begin(); + if (indexed_cmp_.cmp != nullptr) { + current_ = std::upper_bound(indices_.begin(), indices_.end(), target, + indexed_cmp_) - + indices_.begin(); + } else { + current_ = + std::upper_bound(keys_.begin(), keys_.end(), target.ToString()) - + keys_.begin(); + } if (!Valid()) { SeekToLast(); } else { @@ -69,9 +83,14 @@ class VectorIterator : public InternalIterator { virtual bool IsKeyPinned() const override { return true; } virtual bool IsValuePinned() const override { return true; } + protected: + std::vector keys_; + std::vector values_; + size_t current_; + private: struct IndexedKeyComparator { - IndexedKeyComparator(const InternalKeyComparator* c, + IndexedKeyComparator(const Comparator* c, const std::vector* ks) : cmp(c), keys(ks) {} @@ -87,15 +106,12 @@ class VectorIterator : public InternalIterator { return cmp->Compare(a, (*keys)[b]) < 0; } - const InternalKeyComparator* cmp; + const Comparator* cmp; const std::vector* keys; }; - std::vector keys_; - std::vector values_; IndexedKeyComparator indexed_cmp_; std::vector indices_; - size_t current_; }; } // namespace ROCKSDB_NAMESPACE