From 4b50f135401710de452570d7cf8db3722ed88a34 Mon Sep 17 00:00:00 2001
From: sdong <siying.d@fb.com>
Date: Wed, 27 Jan 2016 15:28:15 -0800
Subject: [PATCH] Should not skip bloom filter for L0 during the query.

Summary: It's a regression bug caused by e089db40f9c8f2a8af466377ed0f6fd8a3c26456. With the change, if options.optimize_filters_for_hits=true and there are only L0 files (like single level universal compaction), we skip all the files in L0, which is more than necessary. Fix it by always trying to query bloom filter for files in level 0.

Test Plan: Add a unit test for it.

Reviewers: anthony, rven, yhchiang, IslamAbdelRahman, kradhakrishnan, andrewkr

Reviewed By: andrewkr

Subscribers: leveldb, dhruba

Differential Revision: https://reviews.facebook.net/D53493
---
 db/db_universal_compaction_test.cc | 62 ++++++++++++++++++++++++++++++
 db/version_set.cc                  |  2 +-
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/db/db_universal_compaction_test.cc b/db/db_universal_compaction_test.cc
index 9efcf4ae5..9d3cca83c 100644
--- a/db/db_universal_compaction_test.cc
+++ b/db/db_universal_compaction_test.cc
@@ -14,6 +14,11 @@
 
 namespace rocksdb {
 
+static uint64_t TestGetTickerCount(const Options& options,
+                                   Tickers ticker_type) {
+  return options.statistics->getTickerCount(ticker_type);
+}
+
 static std::string CompressibleString(Random* rnd, int len) {
   std::string r;
   test::CompressibleString(rnd, 0.8, len, &r);
@@ -154,6 +159,63 @@ TEST_P(DBTestUniversalCompaction, UniversalCompactionSingleSortedRun) {
   }
 }
 
+TEST_P(DBTestUniversalCompaction, OptimizeFiltersForHits) {
+  Options options;
+  options = CurrentOptions(options);
+  options.compaction_style = kCompactionStyleUniversal;
+  options.compaction_options_universal.size_ratio = 5;
+  options.num_levels = num_levels_;
+  options.write_buffer_size = 105 << 10;  // 105KB
+  options.arena_block_size = 4 << 10;
+  options.target_file_size_base = 32 << 10;  // 32KB
+  // trigger compaction if there are >= 4 files
+  options.level0_file_num_compaction_trigger = 4;
+  BlockBasedTableOptions bbto;
+  bbto.cache_index_and_filter_blocks = true;
+  bbto.filter_policy.reset(NewBloomFilterPolicy(10, false));
+  bbto.whole_key_filtering = true;
+  options.table_factory.reset(NewBlockBasedTableFactory(bbto));
+  options.optimize_filters_for_hits = true;
+  options.statistics = rocksdb::CreateDBStatistics();
+  options.memtable_factory.reset(new SpecialSkipListFactory(3));
+
+  DestroyAndReopen(options);
+
+  // block compaction from happening
+  env_->SetBackgroundThreads(1, Env::LOW);
+  test::SleepingBackgroundTask sleeping_task_low;
+  env_->Schedule(&test::SleepingBackgroundTask::DoSleepTask, &sleeping_task_low,
+                 Env::Priority::LOW);
+
+  Put("", "");
+  for (int num = 0; num < options.level0_file_num_compaction_trigger; num++) {
+    Put(Key(num * 10), "val");
+    Put(Key(30 + num * 10), "val");
+    Put(Key(60 + num * 10), "val");
+
+    dbfull()->TEST_WaitForFlushMemTable();
+  }
+
+  // Query set of non existing keys
+  for (int i = 5; i < 90; i += 10) {
+    ASSERT_EQ(Get(Key(i)), "NOT_FOUND");
+  }
+
+  // Make sure bloom filter is used at least once.
+  ASSERT_GT(TestGetTickerCount(options, BLOOM_FILTER_USEFUL), 0);
+  auto prev_counter = TestGetTickerCount(options, BLOOM_FILTER_USEFUL);
+
+  // Unblock compaction and wait it for happening.
+  sleeping_task_low.WakeUp();
+  dbfull()->TEST_WaitForCompact();
+
+  // The same queries will not trigger bloom filter
+  for (int i = 5; i < 90; i += 10) {
+    ASSERT_EQ(Get(Key(i)), "NOT_FOUND");
+  }
+  ASSERT_EQ(prev_counter, TestGetTickerCount(options, BLOOM_FILTER_USEFUL));
+}
+
 // TODO(kailiu) The tests on UniversalCompaction has some issues:
 //  1. A lot of magic numbers ("11" or "12").
 //  2. Made assumption on the memtable flush conditions, which may change from
diff --git a/db/version_set.cc b/db/version_set.cc
index 235789512..3679bfbb4 100644
--- a/db/version_set.cc
+++ b/db/version_set.cc
@@ -963,7 +963,7 @@ void Version::Get(const ReadOptions& read_options, const LookupKey& k,
 bool Version::IsFilterSkipped(int level) {
   // Reaching the bottom level implies misses at all upper levels, so we'll
   // skip checking the filters when we predict a hit.
-  return cfd_->ioptions()->optimize_filters_for_hits &&
+  return cfd_->ioptions()->optimize_filters_for_hits && level > 0 &&
          level == storage_info_.num_non_empty_levels() - 1;
 }