From d50619a5590686d0672097c92204aeeae1056d27 Mon Sep 17 00:00:00 2001 From: sdong Date: Tue, 1 Apr 2014 15:00:48 -0700 Subject: [PATCH] PlainTableIterator::Seek() shouldn't check bloom filter in total order mode Summary: In total order mode, iterator's seek() shouldn't check total order. Also some cleaning up about checking null for shared pointers. I don't know the behavior before it. This bug was reported by @igor. Test Plan: test plain_table_db_test Reviewers: ljin, haobo, igor Reviewed By: igor CC: yhchiang, dhruba, igor, leveldb Differential Revision: https://reviews.facebook.net/D17391 --- db/plain_table_db_test.cc | 23 ++++++++++++----------- table/plain_table_reader.cc | 28 +++++++++++----------------- table/plain_table_reader.h | 2 +- 3 files changed, 24 insertions(+), 29 deletions(-) diff --git a/db/plain_table_db_test.cc b/db/plain_table_db_test.cc index 6a3d81aa5..4f1563b94 100644 --- a/db/plain_table_db_test.cc +++ b/db/plain_table_db_test.cc @@ -247,7 +247,7 @@ class TestPlainTableFactory : public PlainTableFactory { }; TEST(PlainTableDBTest, Flush) { - for (int bloom_bits = 0; bloom_bits <= 8; bloom_bits += 8) { + for (int bloom_bits = 0; bloom_bits <= 117; bloom_bits += 117) { for (int total_order = 0; total_order <= 1; total_order++) { Options options = CurrentOptions(); options.create_if_missing = true; @@ -272,7 +272,7 @@ TEST(PlainTableDBTest, Flush) { } TEST(PlainTableDBTest, Flush2) { - for (int bloom_bits = 0; bloom_bits <= 10; bloom_bits += 10) { + for (int bloom_bits = 0; bloom_bits <= 117; bloom_bits += 117) { for (int total_order = 0; total_order <= 1; total_order++) { bool expect_bloom_not_match = false; Options options = CurrentOptions(); @@ -327,7 +327,7 @@ TEST(PlainTableDBTest, Flush2) { } TEST(PlainTableDBTest, Iterator) { - for (int bloom_bits = 0; bloom_bits <= 8; bloom_bits += 8) { + for (int bloom_bits = 0; bloom_bits <= 117; bloom_bits += 117) { for (int total_order = 0; total_order <= 1; total_order++) { bool expect_bloom_not_match = false; Options options = CurrentOptions(); @@ -410,17 +410,18 @@ TEST(PlainTableDBTest, Iterator) { // Test Bloom Filter if (bloom_bits > 0) { - // Neither key nor value should exist. - expect_bloom_not_match = true; - iter->Seek("2not000000000bar"); - ASSERT_TRUE(!iter->Valid()); - - // Key doesn't exist any more but prefix exists. - if (total_order) { + if (!total_order) { + // Neither key nor value should exist. + expect_bloom_not_match = true; iter->Seek("2not000000000bar"); ASSERT_TRUE(!iter->Valid()); + ASSERT_EQ("NOT_FOUND", Get("2not000000000bar")); + expect_bloom_not_match = false; + } else { + expect_bloom_not_match = true; + ASSERT_EQ("NOT_FOUND", Get("2not000000000bar")); + expect_bloom_not_match = false; } - expect_bloom_not_match = false; } delete iter; diff --git a/table/plain_table_reader.cc b/table/plain_table_reader.cc index d521446f8..a6f8bed0e 100644 --- a/table/plain_table_reader.cc +++ b/table/plain_table_reader.cc @@ -267,14 +267,14 @@ Status PlainTableReader::PopulateIndexRecordList(IndexRecordList* record_list, void PlainTableReader::AllocateIndexAndBloom(int num_prefixes) { index_.reset(); - if (options_.prefix_extractor != nullptr) { + if (options_.prefix_extractor.get() != nullptr) { uint32_t bloom_total_bits = num_prefixes * kBloomBitsPerKey; if (bloom_total_bits > 0) { bloom_.reset(new DynamicBloom(bloom_total_bits, options_.bloom_locality)); } } - if (options_.prefix_extractor == nullptr || kHashTableRatio <= 0) { + if (options_.prefix_extractor.get() == nullptr || kHashTableRatio <= 0) { // Fall back to pure binary search if the user fails to specify a prefix // extractor. index_size_ = 1; @@ -366,7 +366,7 @@ void PlainTableReader::FillIndexes( Status PlainTableReader::PopulateIndex() { // options.prefix_extractor is requried for a hash-based look-up. - if (options_.prefix_extractor == nullptr && kHashTableRatio != 0) { + if (options_.prefix_extractor.get() == nullptr && kHashTableRatio != 0) { return Status::NotSupported( "PlainTable requires a prefix extractor enable prefix hash mode."); } @@ -488,7 +488,7 @@ Status PlainTableReader::GetOffset(const Slice& target, const Slice& prefix, } bool PlainTableReader::MatchBloom(uint32_t hash) const { - return bloom_ == nullptr || bloom_->MayContainHash(hash); + return bloom_.get() == nullptr || bloom_->MayContainHash(hash); } Slice PlainTableReader::GetPrefix(const ParsedInternalKey& target) const { @@ -676,20 +676,14 @@ void PlainTableIterator::Seek(const Slice& target) { } Slice prefix_slice = table_->GetPrefix(target); - uint32_t prefix_hash; - uint32_t bloom_hash; - if (table_->IsTotalOrderMode()) { - // The total order mode, there is only one hash bucket 0. The bloom filter - // is checked against the whole user key. - prefix_hash = 0; - bloom_hash = GetSliceHash(table_->GetUserKey(target)); - } else { + uint32_t prefix_hash = 0; + // Bloom filter is ignored in total-order mode. + if (!table_->IsTotalOrderMode()) { prefix_hash = GetSliceHash(prefix_slice); - bloom_hash = prefix_hash; - } - if (!table_->MatchBloom(bloom_hash)) { - offset_ = next_offset_ = table_->data_end_offset_; - return; + if (!table_->MatchBloom(prefix_hash)) { + offset_ = next_offset_ = table_->data_end_offset_; + return; + } } bool prefix_match; status_ = table_->GetOffset(target, prefix_slice, prefix_hash, prefix_match, diff --git a/table/plain_table_reader.h b/table/plain_table_reader.h index a93b3dd35..ac2cb8744 100644 --- a/table/plain_table_reader.h +++ b/table/plain_table_reader.h @@ -247,7 +247,7 @@ class PlainTableReader: public TableReader { } bool IsTotalOrderMode() const { - return (options_.prefix_extractor == nullptr); + return (options_.prefix_extractor.get() == nullptr); } // No copying allowed