Fix & test rocksdb_filterpolicy_create_bloom_full (#6132)

Summary:
Add overrides needed in FilterPolicy wrapper to fix
rocksdb_filterpolicy_create_bloom_full (see issue https://github.com/facebook/rocksdb/issues/6129). Re-enabled
assertion in BloomFilterPolicy::CreateFilter that was being violated.
Expanded c_test to identify Bloom filter implementations by FP counts.
(Without the fix, updated test will trigger assertion and fail otherwise
without the assertion.)

Fixes https://github.com/facebook/rocksdb/issues/6129
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6132

Test Plan: updated c_test, also run under valgrind.

Differential Revision: D18864911

Pulled By: pdillinger

fbshipit-source-id: 08e81d7b5368b08e501cd402ef5583f2650c19fa

db/c.cc

@@ -3048,6 +3048,15 @@ rocksdb_filterpolicy_t* rocksdb_filterpolicy_create_bloom_format(int bits_per_ke
     bool KeyMayMatch(const Slice& key, const Slice& filter) const override {
       return rep_->KeyMayMatch(key, filter);
     }
+    // No need to override GetFilterBitsBuilder if this one is overridden
+    rocksdb::FilterBitsBuilder* GetBuilderWithContext(
+        const rocksdb::FilterBuildingContext& context) const override {
+      return rep_->GetBuilderWithContext(context);
+    }
+    rocksdb::FilterBitsReader* GetFilterBitsReader(
+        const Slice& contents) const override {
+      return rep_->GetFilterBitsReader(contents);
+    }
     static void DoNothing(void*) { }
   };
   Wrapper* wrapper = new Wrapper;

db/c_test.c

@@ -1051,17 +1051,20 @@ int main(int argc, char** argv) {
   }
 
   StartPhase("filter");
-  for (run = 0; run < 2; run++) {
-    // First run uses custom filter, second run uses bloom filter
+  for (run = 0; run <= 2; run++) {
+    // First run uses custom filter
+    // Second run uses old block-based bloom filter
+    // Third run uses full bloom filter
     CheckNoError(err);
     rocksdb_filterpolicy_t* policy;
     if (run == 0) {
       policy = rocksdb_filterpolicy_create(
           NULL, FilterDestroy, FilterCreate, FilterKeyMatch, NULL, FilterName);
+    } else if (run == 1) {
+      policy = rocksdb_filterpolicy_create_bloom(8);
     } else {
-      policy = rocksdb_filterpolicy_create_bloom(10);
+      policy = rocksdb_filterpolicy_create_bloom_full(8);
     }
-
     rocksdb_block_based_options_set_filter_policy(table_options, policy);
 
     // Create new database
@@ -1074,12 +1077,24 @@ int main(int argc, char** argv) {
     CheckNoError(err);
     rocksdb_put(db, woptions, "bar", 3, "barvalue", 8, &err);
     CheckNoError(err);
+
+    {
+      // Add enough keys to get just one reasonably populated Bloom filter
+      const int keys_to_add = 1500;
+      int i;
+      char keybuf[100];
+      for (i = 0; i < keys_to_add; i++) {
+        snprintf(keybuf, sizeof(keybuf), "yes%020d", i);
+        rocksdb_put(db, woptions, keybuf, strlen(keybuf), "val", 3, &err);
+        CheckNoError(err);
+      }
+    }
     rocksdb_compact_range(db, NULL, 0, NULL, 0);
 
     fake_filter_result = 1;
     CheckGet(db, roptions, "foo", "foovalue");
     CheckGet(db, roptions, "bar", "barvalue");
-    if (phase == 0) {
+    if (run == 0) {
       // Must not find value when custom filter returns false
       fake_filter_result = 0;
       CheckGet(db, roptions, "foo", NULL);
@@ -1089,6 +1104,36 @@ int main(int argc, char** argv) {
       CheckGet(db, roptions, "foo", "foovalue");
       CheckGet(db, roptions, "bar", "barvalue");
     }
+
+    {
+      // Query some keys not added to identify Bloom filter implementation
+      // from false positive queries, using perfcontext to detect Bloom
+      // filter behavior
+      rocksdb_perfcontext_t* perf = rocksdb_perfcontext_create();
+      rocksdb_perfcontext_reset(perf);
+
+      const int keys_to_query = 10000;
+      int i;
+      char keybuf[100];
+      for (i = 0; i < keys_to_query; i++) {
+        fake_filter_result = i % 2;
+        snprintf(keybuf, sizeof(keybuf), "no%020d", i);
+        CheckGet(db, roptions, keybuf, NULL);
+      }
+
+      // Other than the first value, these are essentially fingerprints of
+      // the underlying Bloom filter schemas.
+      static const int expected_hits[] = {5000, 241, 224};
+      CheckCondition(
+          expected_hits[run] ==
+          (int)rocksdb_perfcontext_metric(perf, rocksdb_bloom_sst_hit_count));
+      CheckCondition(
+          (keys_to_query - expected_hits[run]) ==
+          (int)rocksdb_perfcontext_metric(perf, rocksdb_bloom_sst_miss_count));
+
+      rocksdb_perfcontext_destroy(perf);
+    }
+
     // Reset the policy
     rocksdb_block_based_options_set_filter_policy(table_options, NULL);
     rocksdb_options_set_block_based_table_factory(options, table_options);

table/block_based/filter_policy.cc

@@ -443,8 +443,7 @@ void BloomFilterPolicy::CreateFilter(const Slice* keys, int n,
                                      std::string* dst) const {
   // We should ideally only be using this deprecated interface for
   // appropriately constructed BloomFilterPolicy
-  // FIXME disabled because of bug in C interface; see issue #6129
-  //assert(mode_ == kDeprecatedBlock);
+  assert(mode_ == kDeprecatedBlock);
 
   // Compute bloom filter size (in both bits and bytes)
   uint32_t bits = static_cast<uint32_t>(n * whole_bits_per_key_);