Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a BoundedIterator class #8309

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Add a BoundedIterator class
The BoundedIterator class extends Iterator and to force the positioning operations to happen between a lower and upper bounds.

This class can be used to implement bounds operations on generic Iterators.
  • Loading branch information
mrambacher committed May 17, 2021
commit a2f45e75781657551337ba334f8488cbee776cc2
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -792,6 +792,7 @@ set(SOURCES
trace_replay/trace_replay.cc
trace_replay/block_cache_tracer.cc
trace_replay/io_tracer.cc
util/bounded_iterator.cc
util/coding.cc
util/compaction_job_stats_impl.cc
util/comparator.cc
Expand Down
2 changes: 2 additions & 0 deletions TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ cpp_library(
"trace_replay/block_cache_tracer.cc",
"trace_replay/io_tracer.cc",
"trace_replay/trace_replay.cc",
"util/bounded_iterator.cc",
"util/build_version.cc",
"util/coding.cc",
"util/compaction_job_stats_impl.cc",
Expand Down Expand Up @@ -643,6 +644,7 @@ cpp_library(
"trace_replay/block_cache_tracer.cc",
"trace_replay/io_tracer.cc",
"trace_replay/trace_replay.cc",
"util/bounded_iterator.cc",
"util/build_version.cc",
"util/coding.cc",
"util/compaction_job_stats_impl.cc",
Expand Down
2 changes: 2 additions & 0 deletions db/arena_wrapped_db_iter.h
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ class ArenaWrappedDBIter : public Iterator {

Status GetProperty(std::string prop_name, std::string* prop) override;

const Slice* lower_bound() const override { return db_iter_->lower_bound(); }
const Slice* upper_bound() const override { return db_iter_->upper_bound(); }
Status Refresh() override;

void Init(Env* env, const ReadOptions& read_options,
Expand Down
2 changes: 2 additions & 0 deletions db/comparator_db_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ class KVIter : public Iterator {
Slice key() const override { return iter_->first; }
Slice value() const override { return iter_->second; }
Status status() const override { return Status::OK(); }
const Slice* lower_bound() const override { return nullptr; }
const Slice* upper_bound() const override { return nullptr; }

private:
const stl_wrappers::KVMap* const map_;
Expand Down
14 changes: 6 additions & 8 deletions db/db_iter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1377,20 +1377,18 @@ void DBIter::SeekForPrev(const Slice& target) {
#ifndef ROCKSDB_LITE
if (db_impl_ != nullptr && cfd_ != nullptr) {
// TODO: What do we do if this returns an error?
Slice lower_bound, upper_bound;
Slice lower, upper;
if (iterate_lower_bound_ != nullptr) {
lower_bound = *iterate_lower_bound_;
lower = *iterate_lower_bound_;
} else {
lower_bound = Slice("");
lower = Slice("");
}
if (iterate_upper_bound_ != nullptr) {
upper_bound = *iterate_upper_bound_;
upper = *iterate_upper_bound_;
} else {
upper_bound = Slice("");
upper = Slice("");
}
db_impl_
->TraceIteratorSeekForPrev(cfd_->GetID(), target, lower_bound,
upper_bound)
db_impl_->TraceIteratorSeekForPrev(cfd_->GetID(), target, lower, upper)
.PermitUncheckedError();
}
#endif // ROCKSDB_LITE
Expand Down
3 changes: 3 additions & 0 deletions db/db_iter.h
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,9 @@ class DBIter final : public Iterator {

Status GetProperty(std::string prop_name, std::string* prop) override;

const Slice* lower_bound() const override { return iterate_lower_bound_; }
const Slice* upper_bound() const override { return iterate_upper_bound_; }

void Next() final override;
void Prev() final override;
// 'target' does not contain timestamp, even if user timestamp feature is
Expand Down
142 changes: 139 additions & 3 deletions db/db_iter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
// COPYING file in the root directory) and Apache 2.0 License
// (found in the LICENSE.Apache file in the root directory).

#include <string>
#include <vector>
#include "db/db_iter.h"

#include <algorithm>
#include <string>
#include <utility>
#include <vector>

#include "db/db_iter.h"
#include "db/dbformat.h"
#include "rocksdb/comparator.h"
#include "rocksdb/options.h"
Expand All @@ -19,6 +20,7 @@
#include "table/merging_iterator.h"
#include "test_util/sync_point.h"
#include "test_util/testharness.h"
#include "util/bounded_iterator.h"
#include "util/string_util.h"
#include "utilities/merge_operators.h"

Expand Down Expand Up @@ -675,6 +677,140 @@ TEST_F(DBIteratorTest, DBIteratorPrevNext) {
}
}

static void CheckIterValue(Iterator* iter, bool valid, const std::string& key) {
ASSERT_EQ(valid, iter->Valid());
if (valid) {
ASSERT_EQ(iter->key().ToString(), key);
ASSERT_EQ(iter->value().ToString(), key);
}
}

TEST_F(DBIteratorTest, BoundedIterator) {
Options options;
ImmutableOptions cf_options = ImmutableOptions(options);
MutableCFOptions mutable_cf_options = MutableCFOptions(options);
const Comparator* comparator = BytewiseComparator();

// 0 means no upper bounds
// 1 means bounds in BoundedIterator but not read options
// 2 means bounds in read options and not BoundedIterator
// 3 means same bounds in both ReadOptions+ BoundedIterator
// 4 means bounds in ReadOptions < BoundedIterator
// 5 means bounds in ReadOptions > BoundedIterator
for (int l = 0; l < 6; l++) {
for (int u = 0; u < 6; u++) {
TestIterator* internal_iter = new TestIterator(BytewiseComparator());
internal_iter->AddPut("c", "c");
internal_iter->AddPut("e", "e");
internal_iter->AddPut("f", "f");
internal_iter->AddPut("h", "h");
internal_iter->AddPut("i", "i");
internal_iter->AddPut("k", "k");
internal_iter->AddPut("l", "l");
internal_iter->AddPut("n", "n");
internal_iter->Finish();

ReadOptions ro;
Slice ek("e");
Slice fk("f");
Slice kk("k");
Slice lk("l");

Slice* lower_bound = nullptr;
Slice* upper_bound = nullptr;
switch (l) {
case 0: // No lower bound
break;
case 1: // Lower bound in read options
ro.iterate_lower_bound = &fk;
break;
case 2: // Lower bound in iterator
lower_bound = &fk;
break;
case 3: // Same lower bound in both
lower_bound = &fk;
ro.iterate_lower_bound = &fk;
break;
case 4:
lower_bound = &fk;
ro.iterate_lower_bound = &ek;
case 5:
lower_bound = &ek;
ro.iterate_lower_bound = &fk;
}
switch (u) {
case 0: // No upper bound
break;
case 1: // Upper bound in read options
ro.iterate_upper_bound = &kk;
break;
case 2: // Upper bound in iterator
upper_bound = &kk;
break;
case 3: // Same upper bound in both
upper_bound = &kk;
ro.iterate_upper_bound = &kk;
break;
case 4:
upper_bound = &lk;
ro.iterate_upper_bound = &kk;
case 5:
upper_bound = &kk;
ro.iterate_upper_bound = &kk;
}

std::unique_ptr<Iterator> bounded_iter(BoundedIterator::Create(
NewDBIterator(env_, ro, cf_options, mutable_cf_options, comparator,
internal_iter, nullptr /* version */, 10 /* sequence */,
options.max_sequential_skip_in_iterations,
nullptr /* read_callback */),
comparator, lower_bound, upper_bound));

bounded_iter->SeekToLast();
CheckIterValue(bounded_iter.get(), true, (u == 0) ? "n" : "i");

bounded_iter->Prev();
CheckIterValue(bounded_iter.get(), true, (u == 0) ? "l" : "h");

bounded_iter->Next();
CheckIterValue(bounded_iter.get(), true, (u == 0) ? "n" : "i");

bounded_iter->Next();
CheckIterValue(bounded_iter.get(), false, "");

bounded_iter->SeekToFirst();
CheckIterValue(bounded_iter.get(), true, (l == 0) ? "c" : "f");

bounded_iter->Next();
CheckIterValue(bounded_iter.get(), true, (l == 0) ? "e" : "h");

bounded_iter->Prev();
CheckIterValue(bounded_iter.get(), true, (l == 0) ? "c" : "f");

bounded_iter->Prev();
CheckIterValue(bounded_iter.get(), false, "");

bounded_iter->Seek("h");
CheckIterValue(bounded_iter.get(), true, "h");

bounded_iter->Seek("c");
CheckIterValue(bounded_iter.get(), true, (l == 0) ? "c" : "f");

bounded_iter->Seek("n");
CheckIterValue(bounded_iter.get(), (u == 0), "n");

bounded_iter->SeekForPrev("k");
CheckIterValue(bounded_iter.get(), true, (u == 0) ? "k" : "i");

bounded_iter->SeekForPrev("n");
CheckIterValue(bounded_iter.get(), true, (u == 0) ? "n" : "i");

bounded_iter->SeekForPrev("c");
CheckIterValue(bounded_iter.get(), (l == 0), "c");
}
}
}

TEST_F(DBIteratorTest, DBIteratorEmpty) {
Options options;
ImmutableOptions cf_options = ImmutableOptions(options);
Expand Down
2 changes: 2 additions & 0 deletions db/db_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3103,6 +3103,8 @@ class ModelDB : public DB {
Slice key() const override { return iter_->first; }
Slice value() const override { return iter_->second; }
Status status() const override { return Status::OK(); }
const Slice* lower_bound() const override { return nullptr; }
const Slice* upper_bound() const override { return nullptr; }

private:
const KVMap* const map_;
Expand Down
20 changes: 20 additions & 0 deletions include/rocksdb/iterator.h
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,26 @@ class Iterator : public Cleanable {
assert(false);
return Slice();
}

/**
* Returns the lower bound if this iterator has a
* ReadOptions::iterate_lower_bound set, else
* returns nullptr.
*
* If a lower bound is present, it should be assumed
* that the iterator adheres to it.
*/
virtual const Slice* lower_bound() const = 0;

/**
* Returns the upper bound if this iterator has a
* ReadOptions::iterate_upper_bound set, else
* returns nullptr.
*
* If an upper bound is present, it should be assumed
* that the iterator adheres to it.
*/
virtual const Slice* upper_bound() const = 0;
};

// Return an empty iterator (yields nothing).
Expand Down
1 change: 1 addition & 0 deletions src.mk
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ LIB_SOURCES = \
trace_replay/trace_replay.cc \
trace_replay/block_cache_tracer.cc \
trace_replay/io_tracer.cc \
util/bounded_iterator.cc \
util/build_version.cc \
util/coding.cc \
util/compaction_job_stats_impl.cc \
Expand Down
2 changes: 2 additions & 0 deletions table/iterator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ class EmptyIterator : public Iterator {
return Slice();
}
Status status() const override { return status_; }
const Slice* lower_bound() const override { return nullptr; }
const Slice* upper_bound() const override { return nullptr; }

private:
Status status_;
Expand Down
Loading