From f2b0514d44f96ccfb195195efd89ed014dc1713b Mon Sep 17 00:00:00 2001
From: Jay Huh <jewoongh@meta.com>
Date: Fri, 15 Nov 2024 13:55:06 -0800
Subject: [PATCH] Fix FilterPolicy Serialization and test

---
 db/db_bloom_filter_test.cc                    | 22 ++++++++++-
 options/cf_options.cc                         | 38 ++++++++++++++-----
 .../block_based/block_based_table_factory.cc  |  2 +-
 3 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/db/db_bloom_filter_test.cc b/db/db_bloom_filter_test.cc
index 66fbaab0290..fd43d370f6b 100644
--- a/db/db_bloom_filter_test.cc
+++ b/db/db_bloom_filter_test.cc
@@ -24,6 +24,7 @@
 #include "rocksdb/perf_context.h"
 #include "rocksdb/statistics.h"
 #include "rocksdb/table.h"
+#include "rocksdb/utilities/object_registry.h"
 #include "table/block_based/block_based_table_reader.h"
 #include "table/block_based/filter_policy_internal.h"
 #include "table/format.h"
@@ -1839,7 +1840,26 @@ class TestingContextCustomFilterPolicy
 }  // anonymous namespace
 
 TEST_F(DBBloomFilterTest, ContextCustomFilterPolicy) {
-  auto policy = std::make_shared<TestingContextCustomFilterPolicy>(15, 8, 5);
+  int bpk_fifo = 15;
+  int bpk_l0_other = 8;
+  int bpk_otherwise = 5;
+  auto policy = std::make_shared<TestingContextCustomFilterPolicy>(
+      bpk_fifo, bpk_l0_other, bpk_otherwise);
+
+  // Little hack to make PersistRocksDBOptions work
+  ObjectRegistry::Default()
+      ->AddLibrary("db_bloom_filter_test")
+      ->AddFactory<FilterPolicy>(
+          policy->Name(),
+          [&bpk_fifo, &bpk_l0_other, &bpk_otherwise](
+              const std::string& /*uri*/, std::unique_ptr<FilterPolicy>* guard,
+              std::string* /* errmsg */) {
+            std::unordered_map<std::string, std::string> map;
+            guard->reset(new LevelAndStyleCustomFilterPolicy(
+                bpk_fifo, bpk_l0_other, bpk_otherwise));
+            return guard->get();
+          });
+
   Options options;
   for (bool fifo : {true, false}) {
     options = CurrentOptions();
diff --git a/options/cf_options.cc b/options/cf_options.cc
index c12b6e77a88..617cf57ba1d 100644
--- a/options/cf_options.cc
+++ b/options/cf_options.cc
@@ -219,6 +219,17 @@ static Status TableFactoryParseFn(const ConfigOptions& opts,
   return s;
 }
 
+static bool TableFactoryEqualsFn(const ConfigOptions& opts,
+                                 const std::string& /*name*/, const void* addr1,
+                                 const void* addr2, std::string* mismatch) {
+  auto table_factory1 =
+      static_cast<const std::shared_ptr<TableFactory>*>(addr1);
+  auto table_factory2 =
+      static_cast<const std::shared_ptr<TableFactory>*>(addr2);
+  return table_factory1->get()->AreEquivalent(opts, table_factory2->get(),
+                                              mismatch);
+}
+
 const std::string kOptNameBMCompOpts = "bottommost_compression_opts";
 const std::string kOptNameCompOpts = "compression_opts";
 
@@ -355,23 +366,32 @@ static std::unordered_map<std::string, OptionTypeInfo>
           OptionTypeFlags::kMutable}},
         {"table_factory",
          {offsetof(struct MutableCFOptions, table_factory),
-          OptionType::kCustomizable, OptionVerificationType::kByName,
-          OptionTypeFlags::kShared | OptionTypeFlags::kCompareLoose |
+          OptionType::kCustomizable,
+          OptionVerificationType::kByName,
+          OptionTypeFlags::kShared | OptionTypeFlags::kCompareDefault |
               OptionTypeFlags::kStringNameOnly | OptionTypeFlags::kDontPrepare |
               OptionTypeFlags::kMutable,
-          TableFactoryParseFn}},
+          TableFactoryParseFn,
+          {} /* SerializeFn */,
+          TableFactoryEqualsFn}},
         {"block_based_table_factory",
          {offsetof(struct MutableCFOptions, table_factory),
-          OptionType::kCustomizable, OptionVerificationType::kAlias,
-          OptionTypeFlags::kShared | OptionTypeFlags::kCompareLoose |
+          OptionType::kCustomizable,
+          OptionVerificationType::kAlias,
+          OptionTypeFlags::kShared | OptionTypeFlags::kCompareDefault |
               OptionTypeFlags::kMutable,
-          TableFactoryParseFn}},
+          TableFactoryParseFn,
+          {} /* SerializeFn */,
+          TableFactoryEqualsFn}},
         {"plain_table_factory",
          {offsetof(struct MutableCFOptions, table_factory),
-          OptionType::kCustomizable, OptionVerificationType::kAlias,
-          OptionTypeFlags::kShared | OptionTypeFlags::kCompareLoose |
+          OptionType::kCustomizable,
+          OptionVerificationType::kAlias,
+          OptionTypeFlags::kShared | OptionTypeFlags::kCompareDefault |
               OptionTypeFlags::kMutable,
-          TableFactoryParseFn}},
+          TableFactoryParseFn,
+          {} /* SerializeFn */,
+          TableFactoryEqualsFn}},
         {"filter_deletes",
          {0, OptionType::kBoolean, OptionVerificationType::kDeprecated,
           OptionTypeFlags::kMutable}},
diff --git a/table/block_based/block_based_table_factory.cc b/table/block_based/block_based_table_factory.cc
index d461dc5269b..821da2d6439 100644
--- a/table/block_based/block_based_table_factory.cc
+++ b/table/block_based/block_based_table_factory.cc
@@ -312,7 +312,7 @@ static struct BlockBasedTableTypeInfo {
         {"filter_policy",
          OptionTypeInfo::AsCustomSharedPtr<const FilterPolicy>(
              offsetof(struct BlockBasedTableOptions, filter_policy),
-             OptionVerificationType::kByNameAllowFromNull)},
+             OptionVerificationType::kByName, OptionTypeFlags::kAllowNull)},
         {"whole_key_filtering",
          {offsetof(struct BlockBasedTableOptions, whole_key_filtering),
           OptionType::kBoolean, OptionVerificationType::kNormal}},