Make FilterPolicy Customizable (facebook#9590)

Summary:
Make FilterPolicy into a Customizable class.  Allow new FilterPolicy to be discovered through the ObjectRegistry

Pull Request resolved: facebook#9590

Reviewed By: pdillinger

Differential Revision: D34327367

Pulled By: mrambacher

fbshipit-source-id: 37e7edac90ec9457422b72f359ab8ef48829c190

HISTORY.md

@@ -43,8 +43,7 @@
   string. Use something like "filter_policy=ribbonfilter:10" instead.
   * Allow configuration string like "filter_policy=bloomfilter:10" without
   bool, to minimize acknowledgement of obsolete block-based filter.
-  * A `filter_policy` loaded from an OPTIONS file can read existing filters
-  but still does not support writing new filters.
+  * Made FilterPolicy Customizable. Configuration of filter_policy is now accurately saved in OPTIONS file and can be loaded with LoadOptionsFromFile. (Loading an OPTIONS file generated by a previous version only enables reading and using existing filters, not generating new filters. Previously, no filter_policy would be configured from a saved OPTIONS file.)
   * Change meaning of nullptr return from GetBuilderWithContext() from "use
     block-based filter" to "generate no filter in this case."
     * Also, when user specifies bits_per_key < 0.5, we now round this down

db/db_bloom_filter_test.cc

@@ -33,14 +33,15 @@ std::shared_ptr<const FilterPolicy> Create(double bits_per_key,
                                            const std::string& name) {
   return BloomLikeFilterPolicy::Create(name, bits_per_key);
 }
-const std::string kLegacyBloom = test::LegacyBloomFilterPolicy::kName();
+const std::string kLegacyBloom = test::LegacyBloomFilterPolicy::kClassName();
 const std::string kDeprecatedBlock =
-    DeprecatedBlockBasedBloomFilterPolicy::kName();
-const std::string kFastLocalBloom = test::FastLocalBloomFilterPolicy::kName();
+    DeprecatedBlockBasedBloomFilterPolicy::kClassName();
+const std::string kFastLocalBloom =
+    test::FastLocalBloomFilterPolicy::kClassName();
 const std::string kStandard128Ribbon =
-    test::Standard128RibbonFilterPolicy::kName();
-const std::string kAutoBloom = BloomFilterPolicy::kName();
-const std::string kAutoRibbon = RibbonFilterPolicy::kName();
+    test::Standard128RibbonFilterPolicy::kClassName();
+const std::string kAutoBloom = BloomFilterPolicy::kClassName();
+const std::string kAutoRibbon = RibbonFilterPolicy::kClassName();
 }  // namespace
 
 // DB tests related to bloom filter.
@@ -593,10 +594,9 @@ class AlwaysTrueBitsBuilder : public FilterBitsBuilder {
   size_t ApproximateNumEntries(size_t) override { return SIZE_MAX; }
 };
 
-class AlwaysTrueFilterPolicy : public BloomLikeFilterPolicy {
+class AlwaysTrueFilterPolicy : public ReadOnlyBuiltinFilterPolicy {
  public:
-  explicit AlwaysTrueFilterPolicy(bool skip)
-      : BloomLikeFilterPolicy(/* ignored */ 10), skip_(skip) {}
+  explicit AlwaysTrueFilterPolicy(bool skip) : skip_(skip) {}
 
   FilterBitsBuilder* GetBuilderWithContext(
       const FilterBuildingContext&) const override {
@@ -607,10 +607,6 @@ class AlwaysTrueFilterPolicy : public BloomLikeFilterPolicy {
     }
   }
 
-  std::string GetId() const override {
-    return "rocksdb.test.AlwaysTrueFilterPolicy";
-  }
-
  private:
   bool skip_;
 };
@@ -1606,11 +1602,11 @@ class TestingContextCustomFilterPolicy
     test_report_ +=
         OptionsHelper::compaction_style_to_string[context.compaction_style];
     test_report_ += ",n=";
-    test_report_ += ToString(context.num_levels);
+    test_report_ += ROCKSDB_NAMESPACE::ToString(context.num_levels);
     test_report_ += ",l=";
-    test_report_ += ToString(context.level_at_creation);
+    test_report_ += ROCKSDB_NAMESPACE::ToString(context.level_at_creation);
     test_report_ += ",b=";
-    test_report_ += ToString(int{context.is_bottommost});
+    test_report_ += ROCKSDB_NAMESPACE::ToString(int{context.is_bottommost});
     test_report_ += ",r=";
     test_report_ += table_file_creation_reason_to_string[context.reason];
     test_report_ += "\n";

include/rocksdb/filter_policy.h

@@ -28,7 +28,7 @@
 #include <vector>
 
 #include "rocksdb/advanced_options.h"
-#include "rocksdb/memory_allocator.h"
+#include "rocksdb/customizable.h"
 #include "rocksdb/status.h"
 #include "rocksdb/types.h"
 
@@ -85,9 +85,10 @@ struct FilterBuildingContext {
 // defer to other built-in policies (see NewBloomFilterPolicy and
 // NewRibbonFilterPolicy) based on the context provided to
 // GetBuilderWithContext.
-class FilterPolicy {
+class FilterPolicy : public Customizable {
  public:
   virtual ~FilterPolicy();
+  static const char* Type() { return "FilterPolicy"; }
 
   // Creates a new FilterPolicy based on the input value string and returns the
   // result The value might be an ID, and ID with properties, or an old-style
@@ -101,12 +102,6 @@ class FilterPolicy {
                                  const std::string& value,
                                  std::shared_ptr<const FilterPolicy>* result);
 
-  // Return the name of this policy.  Note that if the filter encoding
-  // changes in an incompatible way, the name returned by this method
-  // must be changed.  Otherwise, old incompatible filters may be
-  // passed to methods of this type.
-  virtual const char* Name() const = 0;
-
   // Return a new FilterBitsBuilder for constructing full or partitioned
   // filter blocks, or return nullptr to indicate "no filter". Custom
   // implementations should defer to a built-in FilterPolicy to get a

include/rocksdb/utilities/options_type.h

@@ -35,7 +35,6 @@ enum class OptionType {
   kCompactionPri,
   kCompressionType,
   kCompactionStopStyle,
-  kFilterPolicy,
   kChecksumType,
   kEncodingType,
   kEnv,

options/customizable_test.cc

@@ -21,6 +21,7 @@
 #include "rocksdb/convenience.h"
 #include "rocksdb/env_encryption.h"
 #include "rocksdb/file_checksum.h"
+#include "rocksdb/filter_policy.h"
 #include "rocksdb/flush_block_policy.h"
 #include "rocksdb/memory_allocator.h"
 #include "rocksdb/rate_limiter.h"
@@ -31,6 +32,7 @@
 #include "rocksdb/utilities/customizable_util.h"
 #include "rocksdb/utilities/object_registry.h"
 #include "rocksdb/utilities/options_type.h"
+#include "table/block_based/filter_policy_internal.h"
 #include "table/block_based/flush_block_policy.h"
 #include "table/mock_table.h"
 #include "test_util/mock_time_env.h"
@@ -1481,6 +1483,20 @@ class MockRateLimiter : public RateLimiter {
   }
 };
 
+class MockFilterPolicy : public FilterPolicy {
+ public:
+  static const char* kClassName() { return "MockFilterPolicy"; }
+  const char* Name() const override { return kClassName(); }
+  FilterBitsBuilder* GetBuilderWithContext(
+      const FilterBuildingContext&) const override {
+    return nullptr;
+  }
+  FilterBitsReader* GetFilterBitsReader(
+      const Slice& /*contents*/) const override {
+    return nullptr;
+  }
+};
+
 #ifndef ROCKSDB_LITE
 static int RegisterLocalObjects(ObjectLibrary& library,
                                 const std::string& /*arg*/) {
@@ -1605,6 +1621,14 @@ static int RegisterLocalObjects(ObjectLibrary& library,
         return guard->get();
       });
 
+  library.AddFactory<const FilterPolicy>(
+      MockFilterPolicy::kClassName(),
+      [](const std::string& /*uri*/, std::unique_ptr<const FilterPolicy>* guard,
+         std::string* /* errmsg */) {
+        guard->reset(new MockFilterPolicy());
+        return guard->get();
+      });
+
   return static_cast<int>(library.GetFactoryCount(&num_types));
 }
 #endif  // !ROCKSDB_LITE
@@ -2114,6 +2138,55 @@ TEST_F(LoadCustomizableTest, LoadRateLimiterTest) {
 #endif  // ROCKSDB_LITE
 }
 
+TEST_F(LoadCustomizableTest, LoadFilterPolicyTest) {
+  std::shared_ptr<TableFactory> table;
+  std::shared_ptr<const FilterPolicy> result;
+  ASSERT_NOK(FilterPolicy::CreateFromString(
+      config_options_, MockFilterPolicy::kClassName(), &result));
+
+  ASSERT_OK(FilterPolicy::CreateFromString(config_options_, "", &result));
+  ASSERT_EQ(result, nullptr);
+  ASSERT_OK(FilterPolicy::CreateFromString(
+      config_options_, ReadOnlyBuiltinFilterPolicy::kClassName(), &result));
+  ASSERT_NE(result, nullptr);
+  ASSERT_STREQ(result->Name(), ReadOnlyBuiltinFilterPolicy::kClassName());
+
+#ifndef ROCKSDB_LITE
+  std::string table_opts = "id=BlockBasedTable; filter_policy=";
+  ASSERT_OK(TableFactory::CreateFromString(config_options_,
+                                           table_opts + "nullptr", &table));
+  ASSERT_NE(table.get(), nullptr);
+  auto bbto = table->GetOptions<BlockBasedTableOptions>();
+  ASSERT_NE(bbto, nullptr);
+  ASSERT_EQ(bbto->filter_policy.get(), nullptr);
+  ASSERT_OK(TableFactory::CreateFromString(
+      config_options_, table_opts + ReadOnlyBuiltinFilterPolicy::kClassName(),
+      &table));
+  bbto = table->GetOptions<BlockBasedTableOptions>();
+  ASSERT_NE(bbto, nullptr);
+  ASSERT_NE(bbto->filter_policy.get(), nullptr);
+  ASSERT_STREQ(bbto->filter_policy->Name(),
+               ReadOnlyBuiltinFilterPolicy::kClassName());
+  ASSERT_OK(TableFactory::CreateFromString(
+      config_options_, table_opts + MockFilterPolicy::kClassName(), &table));
+  bbto = table->GetOptions<BlockBasedTableOptions>();
+  ASSERT_NE(bbto, nullptr);
+  ASSERT_EQ(bbto->filter_policy.get(), nullptr);
+  if (RegisterTests("Test")) {
+    ASSERT_OK(FilterPolicy::CreateFromString(
+        config_options_, MockFilterPolicy::kClassName(), &result));
+    ASSERT_NE(result, nullptr);
+    ASSERT_STREQ(result->Name(), MockFilterPolicy::kClassName());
+    ASSERT_OK(TableFactory::CreateFromString(
+        config_options_, table_opts + MockFilterPolicy::kClassName(), &table));
+    bbto = table->GetOptions<BlockBasedTableOptions>();
+    ASSERT_NE(bbto, nullptr);
+    ASSERT_NE(bbto->filter_policy.get(), nullptr);
+    ASSERT_STREQ(bbto->filter_policy->Name(), MockFilterPolicy::kClassName());
+  }
+#endif  // ROCKSDB_LITE
+}
+
 TEST_F(LoadCustomizableTest, LoadFlushBlockPolicyFactoryTest) {
   std::shared_ptr<TableFactory> table;
   std::shared_ptr<FlushBlockPolicyFactory> result;

options/options_helper.cc

@@ -512,12 +512,6 @@ bool SerializeSingleOptionHelper(const void* opt_address,
           compression_type_string_map,
           *(static_cast<const CompressionType*>(opt_address)), value);
       break;
-    case OptionType::kFilterPolicy: {
-      const auto* ptr =
-          static_cast<const std::shared_ptr<FilterPolicy>*>(opt_address);
-      *value = ptr->get() ? ptr->get()->Name() : kNullptrString;
-      break;
-    }
     case OptionType::kChecksumType:
       return SerializeEnum<ChecksumType>(
           checksum_type_string_map,

options/options_test.cc

@@ -844,6 +844,7 @@ TEST_F(OptionsTest, GetBlockBasedTableOptionsFromString) {
   ConfigOptions config_options;
   config_options.input_strings_escaped = false;
   config_options.ignore_unknown_options = false;
+  config_options.ignore_unsupported_options = false;
 
   // make sure default values are overwritten by something else
   ASSERT_OK(GetBlockBasedTableOptionsFromString(
@@ -878,8 +879,8 @@ TEST_F(OptionsTest, GetBlockBasedTableOptionsFromString) {
   ASSERT_EQ(new_opt.detect_filter_construct_corruption, true);
   ASSERT_EQ(new_opt.reserve_table_builder_memory, true);
   ASSERT_TRUE(new_opt.filter_policy != nullptr);
-  const BloomFilterPolicy* bfp =
-      dynamic_cast<const BloomFilterPolicy*>(new_opt.filter_policy.get());
+  auto bfp = new_opt.filter_policy->CheckedCast<BloomFilterPolicy>();
+  ASSERT_NE(bfp, nullptr);
   EXPECT_EQ(bfp->GetMillibitsPerKey(), 4567);
   EXPECT_EQ(bfp->GetWholeBitsPerKey(), 5);
   // Verify that only the lower 32bits are stored in
@@ -1104,6 +1105,25 @@ TEST_F(OptionsTest, GetBlockBasedTableOptionsFromString) {
   ASSERT_EQ(std::dynamic_pointer_cast<LRUCache>(new_opt.block_cache_compressed)
                 ->GetHighPriPoolRatio(),
             0.5);
+
+  ASSERT_OK(GetBlockBasedTableOptionsFromString(
+      config_options, table_opt, "filter_policy=rocksdb.BloomFilter:1.234",
+      &new_opt));
+  ASSERT_TRUE(new_opt.filter_policy != nullptr);
+  ASSERT_TRUE(
+      new_opt.filter_policy->IsInstanceOf(BloomFilterPolicy::kClassName()));
+  ASSERT_TRUE(
+      new_opt.filter_policy->IsInstanceOf(BloomFilterPolicy::kNickName()));
+
+  // Ribbon filter policy alternative name
+  ASSERT_OK(GetBlockBasedTableOptionsFromString(
+      config_options, table_opt, "filter_policy=rocksdb.RibbonFilter:6.789:5;",
+      &new_opt));
+  ASSERT_TRUE(new_opt.filter_policy != nullptr);
+  ASSERT_TRUE(
+      new_opt.filter_policy->IsInstanceOf(RibbonFilterPolicy::kClassName()));
+  ASSERT_TRUE(
+      new_opt.filter_policy->IsInstanceOf(RibbonFilterPolicy::kNickName()));
 }
 #endif  // !ROCKSDB_LITE
 

table/block_based/block_based_table_factory.cc

@@ -314,44 +314,10 @@ static std::unordered_map<std::string, OptionTypeInfo>
           OptionType::kBoolean, OptionVerificationType::kNormal,
           OptionTypeFlags::kNone}},
         {"filter_policy",
-         {offsetof(struct BlockBasedTableOptions, filter_policy),
-          OptionType::kUnknown, OptionVerificationType::kByNameAllowFromNull,
-          OptionTypeFlags::kNone,
-          // Parses the Filter policy
-          [](const ConfigOptions& opts, const std::string&,
-             const std::string& value, void* addr) {
-            auto* policy =
-                static_cast<std::shared_ptr<const FilterPolicy>*>(addr);
-            return FilterPolicy::CreateFromString(opts, value, policy);
-          },
-          // Converts the FilterPolicy to its string representation
-          [](const ConfigOptions&, const std::string&, const void* addr,
-             std::string* value) {
-            const auto* policy =
-                static_cast<const std::shared_ptr<const FilterPolicy>*>(addr);
-            if (policy->get()) {
-              *value = (*policy)->Name();
-            } else {
-              *value = kNullptrString;
-            }
-            return Status::OK();
-          },
-          // Compares two FilterPolicy objects for equality
-          [](const ConfigOptions&, const std::string&, const void* addr1,
-             const void* addr2, std::string*) {
-            const auto* policy1 =
-                static_cast<const std::shared_ptr<const FilterPolicy>*>(addr1)
-                    ->get();
-            const auto* policy2 =
-                static_cast<const std::shared_ptr<FilterPolicy>*>(addr2)->get();
-            if (policy1 == policy2) {
-              return true;
-            } else if (policy1 != nullptr && policy2 != nullptr) {
-              return (strcmp(policy1->Name(), policy2->Name()) == 0);
-            } else {
-              return false;
-            }
-          }}},
+         OptionTypeInfo::AsCustomSharedPtr<const FilterPolicy>(
+             offsetof(struct BlockBasedTableOptions, filter_policy),
+             OptionVerificationType::kByNameAllowFromNull,
+             OptionTypeFlags::kNone)},
         {"whole_key_filtering",
          {offsetof(struct BlockBasedTableOptions, whole_key_filtering),
           OptionType::kBoolean, OptionVerificationType::kNormal,
@@ -913,6 +879,8 @@ Status GetBlockBasedTableOptionsFromString(
   config_options.input_strings_escaped = false;
   config_options.ignore_unknown_options = false;
   config_options.invoke_prepare_options = false;
+  config_options.ignore_unsupported_options = false;
+
   return GetBlockBasedTableOptionsFromString(config_options, table_options,
                                              opts_str, new_table_options);
 }