Avoid reloading filter on Get() if cache_index_and_filter_blocks == false

Summary:
This fixes the case that filter policy is missing in SST file, but we
open the table with filter policy on and cache_index_and_filter_blocks =
false. The current behavior is that we will try to load it every time on
Get() but fail.

Test Plan: unit test

Reviewers: yhchiang, igor, rven, sdong

Reviewed By: sdong

Subscribers: leveldb

Differential Revision: https://reviews.facebook.net/D25455

table/block_based_table_factory.cc

@@ -71,6 +71,11 @@ Status BlockBasedTableFactory::SanitizeOptions(
     return Status::InvalidArgument("Hash index is specified for block-based "
         "table, but prefix_extractor is not given");
   }
+  if (table_options_.cache_index_and_filter_blocks &&
+      table_options_.no_block_cache) {
+    return Status::InvalidArgument("Enable cache_index_and_filter_blocks, "
+        ", but block cache is disabled");
+  }
   return Status::OK();
 }
 

table/block_based_table_reader.cc

@@ -483,8 +483,8 @@ Status BlockBasedTable::Open(const ImmutableCFOptions& ioptions,
   }
 
   // Will use block cache for index/filter blocks access?
-  if (table_options.block_cache &&
-      table_options.cache_index_and_filter_blocks) {
+  if (table_options.cache_index_and_filter_blocks) {
+    assert(table_options.block_cache != nullptr);
     // Hack: Call NewIndexIterator() to implicitly add index to the block_cache
     unique_ptr<Iterator> iter(new_table->NewIndexIterator(ReadOptions()));
     s = iter->status();
@@ -506,19 +506,7 @@ Status BlockBasedTable::Open(const ImmutableCFOptions& ioptions,
 
       // Set filter block
       if (rep->filter_policy) {
-        // First try reading full_filter, then reading block_based_filter
-        for (auto filter_block_prefix : { kFullFilterBlockPrefix,
-                                          kFilterBlockPrefix }) {
-          std::string key = filter_block_prefix;
-          key.append(rep->filter_policy->Name());
-
-          BlockHandle handle;
-          if (FindMetaBlock(meta_iter.get(), key, &handle).ok()) {
-            rep->filter.reset(ReadFilter(handle, rep,
-                filter_block_prefix, nullptr));
-            break;
-          }
-        }
+        rep->filter.reset(ReadFilter(rep, meta_iter.get(), nullptr));
       }
     } else {
       delete index_reader;
@@ -726,33 +714,43 @@ Status BlockBasedTable::PutDataBlockToCache(
 }
 
 FilterBlockReader* BlockBasedTable::ReadFilter(
-    const BlockHandle& filter_handle, BlockBasedTable::Rep* rep,
-    const std::string& filter_block_prefix, size_t* filter_size) {
+    Rep* rep, Iterator* meta_index_iter, size_t* filter_size) {
   // TODO: We might want to unify with ReadBlockFromFile() if we start
   // requiring checksum verification in Table::Open.
-  ReadOptions opt;
-  BlockContents block;
-  if (!ReadBlockContents(rep->file.get(), rep->footer, opt, filter_handle,
-                         &block, rep->ioptions.env, false).ok()) {
-    return nullptr;
-  }
+  for (auto prefix : {kFullFilterBlockPrefix, kFilterBlockPrefix}) {
+    std::string filter_block_key = prefix;
+    filter_block_key.append(rep->filter_policy->Name());
+    BlockHandle handle;
+    if (FindMetaBlock(meta_index_iter, filter_block_key, &handle).ok()) {
+      BlockContents block;
+      if (!ReadBlockContents(rep->file.get(), rep->footer, ReadOptions(),
+                             handle, &block, rep->ioptions.env, false).ok()) {
+        // Error reading the block
+        return nullptr;
+      }
 
-  if (filter_size) {
-    *filter_size = block.data.size();
-  }
+      if (filter_size) {
+        *filter_size = block.data.size();
+      }
 
-  assert(rep->filter_policy);
-  if (kFilterBlockPrefix == filter_block_prefix) {
-    return new BlockBasedFilterBlockReader(
-        rep->ioptions.prefix_extractor, rep->table_options, std::move(block));
-  } else if (kFullFilterBlockPrefix == filter_block_prefix) {
-    auto filter_bits_reader = rep->filter_policy->
-        GetFilterBitsReader(block.data);
-
-    if (filter_bits_reader != nullptr) {
-      return new FullFilterBlockReader(rep->ioptions.prefix_extractor,
-                                       rep->table_options, std::move(block),
-                                       filter_bits_reader);
+      assert(rep->filter_policy);
+      if (kFilterBlockPrefix == prefix) {
+        return new BlockBasedFilterBlockReader(
+            rep->ioptions.prefix_extractor, rep->table_options,
+            std::move(block));
+      } else if (kFullFilterBlockPrefix == prefix) {
+        auto filter_bits_reader = rep->filter_policy->
+            GetFilterBitsReader(block.data);
+        if (filter_bits_reader != nullptr) {
+          return new FullFilterBlockReader(rep->ioptions.prefix_extractor,
+                                           rep->table_options,
+                                           std::move(block),
+                                           filter_bits_reader);
+        }
+      } else {
+        assert(false);
+        return nullptr;
+      }
     }
   }
   return nullptr;
@@ -760,8 +758,11 @@ FilterBlockReader* BlockBasedTable::ReadFilter(
 
 BlockBasedTable::CachableEntry<FilterBlockReader> BlockBasedTable::GetFilter(
                                                           bool no_io) const {
-  // filter pre-populated
-  if (rep_->filter != nullptr) {
+  // If cache_index_and_filter_blocks is false, filter should be pre-populated.
+  // We will return rep_->filter anyway. rep_->filter can be nullptr if filter
+  // read fails at Open() time. We don't want to reload again since it will
+  // most probably fail again.
+  if (!rep_->table_options.cache_index_and_filter_blocks) {
     return {rep_->filter.get(), nullptr /* cache handle */};
   }
 
@@ -775,8 +776,7 @@ BlockBasedTable::CachableEntry<FilterBlockReader> BlockBasedTable::GetFilter(
   char cache_key[kMaxCacheKeyPrefixSize + kMaxVarint64Length];
   auto key = GetCacheKey(rep_->cache_key_prefix, rep_->cache_key_prefix_size,
                          rep_->footer.metaindex_handle(),
-                         cache_key
-  );
+                         cache_key);
 
   Statistics* statistics = rep_->ioptions.statistics;
   auto cache_handle =
@@ -797,22 +797,12 @@ BlockBasedTable::CachableEntry<FilterBlockReader> BlockBasedTable::GetFilter(
     auto s = ReadMetaBlock(rep_, &meta, &iter);
 
     if (s.ok()) {
-      // First try reading full_filter, then reading block_based_filter
-      for (auto filter_block_prefix : {kFullFilterBlockPrefix,
-                                       kFilterBlockPrefix}) {
-        std::string filter_block_key = filter_block_prefix;
-        filter_block_key.append(rep_->filter_policy->Name());
-        BlockHandle handle;
-        if (FindMetaBlock(iter.get(), filter_block_key, &handle).ok()) {
-          filter = ReadFilter(handle, rep_, filter_block_prefix, &filter_size);
-
-          if (filter == nullptr) break;  // err happen in ReadFilter
-          assert(filter_size > 0);
-          cache_handle = block_cache->Insert(
-              key, filter, filter_size, &DeleteCachedEntry<FilterBlockReader>);
-          RecordTick(statistics, BLOCK_CACHE_ADD);
-          break;
-        }
+      filter = ReadFilter(rep_, iter.get(), &filter_size);
+      if (filter != nullptr) {
+        assert(filter_size > 0);
+        cache_handle = block_cache->Insert(
+            key, filter, filter_size, &DeleteCachedEntry<FilterBlockReader>);
+        RecordTick(statistics, BLOCK_CACHE_ADD);
       }
     }
   }

table/block_based_table_reader.h

@@ -183,10 +183,10 @@ class BlockBasedTable : public TableReader {
       std::unique_ptr<Iterator>* iter);
 
   // Create the filter from the filter block.
-  static FilterBlockReader* ReadFilter(const BlockHandle& filter_handle,
-                                       Rep* rep,
-                                       const std::string& filter_block_prefix,
-                                       size_t* filter_size = nullptr);
+  static FilterBlockReader* ReadFilter(
+      Rep* rep,
+      Iterator* meta_index_iter,
+      size_t* filter_size = nullptr);
 
   static void SetupCacheKeyPrefix(Rep* rep);
 

table/table_test.cc

@@ -1461,8 +1461,6 @@ TEST(BlockBasedTableTest, BlockCacheDisabledTest) {
   options.create_if_missing = true;
   options.statistics = CreateDBStatistics();
   BlockBasedTableOptions table_options;
-  // Intentionally commented out: table_options.cache_index_and_filter_blocks =
-  // true;
   table_options.block_cache = NewLRUCache(1024);
   table_options.filter_policy.reset(NewBloomFilterPolicy(10));
   options.table_factory.reset(new BlockBasedTableFactory(table_options));
@@ -1521,7 +1519,7 @@ TEST(BlockBasedTableTest, FilterBlockInBlockCache) {
   c.Finish(options, ioptions, table_options,
            GetPlainInternalComparator(options.comparator), &keys, &kvmap);
   // preloading filter/index blocks is prohibited.
-  auto reader = dynamic_cast<BlockBasedTable*>(c.GetTableReader());
+  auto* reader = dynamic_cast<BlockBasedTable*>(c.GetTableReader());
   ASSERT_TRUE(!reader->TEST_filter_block_preloaded());
   ASSERT_TRUE(!reader->TEST_index_reader_preloaded());
 
@@ -1567,28 +1565,11 @@ TEST(BlockBasedTableTest, FilterBlockInBlockCache) {
   // release the iterator so that the block cache can reset correctly.
   iter.reset();
 
-  // -- PART 2: Open without block cache
-  table_options.no_block_cache = true;
-  table_options.block_cache.reset();
-  options.table_factory.reset(new BlockBasedTableFactory(table_options));
-  options.statistics = CreateDBStatistics();  // reset the stats
-  const ImmutableCFOptions ioptions1(options);
-  c.Reopen(ioptions1);
-  table_options.no_block_cache = false;
-
-  {
-    iter.reset(c.NewIterator());
-    iter->SeekToFirst();
-    ASSERT_EQ("key", iter->key().ToString());
-    BlockCachePropertiesSnapshot props(options.statistics.get());
-    // Nothing is affected at all
-    props.AssertEqual(0, 0, 0, 0);
-  }
-
-  // -- PART 3: Open with very small block cache
+  // -- PART 2: Open with very small block cache
   // In this test, no block will ever get hit since the block cache is
   // too small to fit even one entry.
   table_options.block_cache = NewLRUCache(1);
+  options.statistics = CreateDBStatistics();
   options.table_factory.reset(new BlockBasedTableFactory(table_options));
   const ImmutableCFOptions ioptions2(options);
   c.Reopen(ioptions2);
@@ -1598,7 +1579,6 @@ TEST(BlockBasedTableTest, FilterBlockInBlockCache) {
                       0, 0, 0);
   }
 
-
   {
     // Both index and data block get accessed.
     // It first cache index block then data block. But since the cache size
@@ -1618,6 +1598,33 @@ TEST(BlockBasedTableTest, FilterBlockInBlockCache) {
     props.AssertEqual(2, 0, 0 + 1,  // data block miss
                       0);
   }
+  iter.reset();
+
+  // -- PART 3: Open table with bloom filter enabled but not in SST file
+  table_options.block_cache = NewLRUCache(4096);
+  table_options.cache_index_and_filter_blocks = false;
+  options.table_factory.reset(NewBlockBasedTableFactory(table_options));
+
+  TableConstructor c3(BytewiseComparator());
+  c3.Add("k01", "hello");
+  ImmutableCFOptions ioptions3(options);
+  // Generate table without filter policy
+  c3.Finish(options, ioptions3, table_options,
+           GetPlainInternalComparator(options.comparator), &keys, &kvmap);
+  // Open table with filter policy
+  table_options.filter_policy.reset(NewBloomFilterPolicy(1));
+  options.table_factory.reset(new BlockBasedTableFactory(table_options));
+  options.statistics = CreateDBStatistics();
+  ImmutableCFOptions ioptions4(options);
+  ASSERT_OK(c3.Reopen(ioptions4));
+  reader = dynamic_cast<BlockBasedTable*>(c3.GetTableReader());
+  ASSERT_TRUE(!reader->TEST_filter_block_preloaded());
+  GetContext get_context(options.comparator, nullptr, nullptr, nullptr,
+                         GetContext::kNotFound, Slice(), nullptr,
+                         nullptr, nullptr);
+  ASSERT_OK(reader->Get(ReadOptions(), "k01", &get_context));
+  BlockCachePropertiesSnapshot props(options.statistics.get());
+  props.AssertFilterBlockStat(0, 0);
 }
 
 TEST(BlockBasedTableTest, BlockCacheLeak) {