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

@@ -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;

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,

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