Revert "Make Comparator into a Customizable Object (#8336)"

This reverts commit 6ad0810.
facebook · Jun 23, 2021 · 6ee0acd · 6ee0acd
1 parent 6fc2818
commit 6ee0acd
Show file tree

Hide file tree

Showing 8 changed files with 44 additions and 152 deletions.
diff --git a/include/rocksdb/comparator.h b/include/rocksdb/comparator.h
@@ -10,7 +10,6 @@
 
 #include <string>
 
-#include "rocksdb/customizable.h"
 #include "rocksdb/rocksdb_namespace.h"
 
 namespace ROCKSDB_NAMESPACE {
@@ -21,7 +20,7 @@ class Slice;
 // used as keys in an sstable or a database.  A Comparator implementation
 // must be thread-safe since rocksdb may invoke its methods concurrently
 // from multiple threads.
-class Comparator : public Customizable {
+class Comparator {
  public:
   Comparator() : timestamp_size_(0) {}
 
@@ -38,11 +37,7 @@ class Comparator : public Customizable {
 
   virtual ~Comparator() {}
 
-  static Status CreateFromString(const ConfigOptions& opts,
-                                 const std::string& id,
-                                 const Comparator** comp);
   static const char* Type() { return "Comparator"; }
-
   // Three-way comparison.  Returns value:
   //   < 0 iff "a" < "b",
   //   == 0 iff "a" == "b",

diff --git a/include/rocksdb/configurable.h b/include/rocksdb/configurable.h
@@ -8,7 +8,6 @@
 
 #pragma once
 
-#include <atomic>
 #include <string>
 #include <unordered_map>
 #include <unordered_set>
@@ -270,7 +269,7 @@ class Configurable {
  protected:
   // True once the object is prepared.  Once the object is prepared, only
   // mutable options can be configured.
-  std::atomic<bool> prepared_;
+  bool prepared_;
 
   // Returns the raw pointer for the associated named option.
   // The name is typically the name of an option registered via the

diff --git a/include/rocksdb/utilities/options_type.h b/include/rocksdb/utilities/options_type.h
@@ -35,6 +35,7 @@ enum class OptionType {
   kCompactionPri,
   kSliceTransform,
   kCompressionType,
+  kComparator,
   kCompactionFilter,
   kCompactionFilterFactory,
   kCompactionStopStyle,

diff --git a/options/cf_options.cc b/options/cf_options.cc
@@ -535,30 +535,22 @@ static std::unordered_map<std::string, OptionTypeInfo>
              OptionVerificationType::kNormal, OptionTypeFlags::kNone,
              {0, OptionType::kCompressionType})},
         {"comparator",
-         OptionTypeInfo::AsCustomRawPtr<const Comparator>(
-             offset_of(&ImmutableCFOptions::user_comparator),
-             OptionVerificationType::kByName, OptionTypeFlags::kCompareLoose,
-             // Serializes a Comparator
-             [](const ConfigOptions& /*opts*/, const std::string&,
-                const void* addr, std::string* value) {
-               // it's a const pointer of const Comparator*
-               const auto* ptr = static_cast<const Comparator* const*>(addr);
-
-               // Since the user-specified comparator will be wrapped by
-               // InternalKeyComparator, we should persist the user-specified
-               // one instead of InternalKeyComparator.
-               if (*ptr == nullptr) {
-                 *value = kNullptrString;
-               } else {
-                 const Comparator* root_comp = (*ptr)->GetRootComparator();
-                 if (root_comp == nullptr) {
-                   root_comp = (*ptr);
-                 }
-                 *value = root_comp->Name();
-               }
-               return Status::OK();
-             },
-             /* Use the default match function*/ nullptr)},
+         {offset_of(&ImmutableCFOptions::user_comparator),
+          OptionType::kComparator, OptionVerificationType::kByName,
+          OptionTypeFlags::kCompareLoose,
+          // Parses the string and sets the corresponding comparator
+          [](const ConfigOptions& opts, const std::string& /*name*/,
+             const std::string& value, void* addr) {
+            auto old_comparator = static_cast<const Comparator**>(addr);
+            const Comparator* new_comparator = *old_comparator;
+            Status status =
+                opts.registry->NewStaticObject(value, &new_comparator);
+            if (status.ok()) {
+              *old_comparator = new_comparator;
+              return status;
+            }
+            return Status::OK();
+          }}},
         {"memtable_insert_with_hint_prefix_extractor",
          {offset_of(
               &ImmutableCFOptions::memtable_insert_with_hint_prefix_extractor),

diff --git a/options/customizable_test.cc b/options/customizable_test.cc
@@ -771,15 +771,6 @@ static int RegisterTestObjects(ObjectLibrary& library,
         guard->reset(new mock::MockTableFactory());
         return guard->get();
       });
-  library.Register<const Comparator>(
-      test::SimpleSuffixReverseComparator::kClassName(),
-      [](const std::string& /*uri*/,
-         std::unique_ptr<const Comparator>* /*guard*/,
-         std::string* /* errmsg */) {
-        static test::SimpleSuffixReverseComparator ssrc;
-        return &ssrc;
-      });
-
   return static_cast<int>(library.GetFactoryCount(&num_types));
 }
 
@@ -789,7 +780,6 @@ static int RegisterLocalObjects(ObjectLibrary& library,
   // Load any locally defined objects here
   return static_cast<int>(library.GetFactoryCount(&num_types));
 }
-#endif  // !ROCKSDB_LITE
 
 class LoadCustomizableTest : public testing::Test {
  public:
@@ -829,31 +819,7 @@ TEST_F(LoadCustomizableTest, LoadTableFactoryTest) {
     ASSERT_STREQ(factory->Name(), "MockTable");
   }
 }
-
-TEST_F(LoadCustomizableTest, LoadComparatorTest) {
-  const Comparator* bytewise = BytewiseComparator();
-  const Comparator* reverse = ReverseBytewiseComparator();
-
-  const Comparator* result = nullptr;
-  ASSERT_NOK(Comparator::CreateFromString(
-      config_options_, test::SimpleSuffixReverseComparator::kClassName(),
-      &result));
-  ASSERT_OK(
-      Comparator::CreateFromString(config_options_, bytewise->Name(), &result));
-  ASSERT_EQ(result, bytewise);
-  ASSERT_OK(
-      Comparator::CreateFromString(config_options_, reverse->Name(), &result));
-  ASSERT_EQ(result, reverse);
-
-  if (RegisterTests("Test")) {
-    ASSERT_OK(Comparator::CreateFromString(
-        config_options_, test::SimpleSuffixReverseComparator::kClassName(),
-        &result));
-    ASSERT_NE(result, nullptr);
-    ASSERT_STREQ(result->Name(),
-                 test::SimpleSuffixReverseComparator::kClassName());
-  }
-}
+#endif  // !ROCKSDB_LITE
 
 }  // namespace ROCKSDB_NAMESPACE
 int main(int argc, char** argv) {

diff --git a/options/options_helper.cc b/options/options_helper.cc
@@ -562,6 +562,23 @@ bool SerializeSingleOptionHelper(const void* opt_address,
                                           : kNullptrString;
       break;
     }
+    case OptionType::kComparator: {
+      // it's a const pointer of const Comparator*
+      const auto* ptr = static_cast<const Comparator* const*>(opt_address);
+      // Since the user-specified comparator will be wrapped by
+      // InternalKeyComparator, we should persist the user-specified one
+      // instead of InternalKeyComparator.
+      if (*ptr == nullptr) {
+        *value = kNullptrString;
+      } else {
+        const Comparator* root_comp = (*ptr)->GetRootComparator();
+        if (root_comp == nullptr) {
+          root_comp = (*ptr);
+        }
+        *value = root_comp->Name();
+      }
+      break;
+    }
     case OptionType::kCompactionFilter: {
       // it's a const pointer of const CompactionFilter*
       const auto* ptr =

diff --git a/test_util/testutil.h b/test_util/testutil.h
@@ -98,8 +98,10 @@ class PlainInternalKeyComparator : public InternalKeyComparator {
 class SimpleSuffixReverseComparator : public Comparator {
  public:
   SimpleSuffixReverseComparator() {}
-  static const char* kClassName() { return "SimpleSuffixReverseComparator"; }
-  virtual const char* Name() const override { return kClassName(); }
+
+  virtual const char* Name() const override {
+    return "SimpleSuffixReverseComparator";
+  }
 
   virtual int Compare(const Slice& a, const Slice& b) const override {
     Slice prefix_a = Slice(a.data(), 8);

diff --git a/util/comparator.cc b/util/comparator.cc
@@ -8,26 +8,20 @@
 // found in the LICENSE file. See the AUTHORS file for names of contributors.
 
 #include "rocksdb/comparator.h"
-
 #include <stdint.h>
-
 #include <algorithm>
 #include <memory>
-#include <mutex>
-
-#include "options/configurable_helper.h"
 #include "port/port.h"
 #include "rocksdb/slice.h"
-#include "rocksdb/utilities/object_registry.h"
 
 namespace ROCKSDB_NAMESPACE {
 
 namespace {
 class BytewiseComparatorImpl : public Comparator {
  public:
   BytewiseComparatorImpl() { }
-  static const char* kClassName() { return "leveldb.BytewiseComparator"; }
-  const char* Name() const override { return kClassName(); }
+
+  const char* Name() const override { return "leveldb.BytewiseComparator"; }
 
   int Compare(const Slice& a, const Slice& b) const override {
     return a.compare(b);
@@ -145,10 +139,9 @@ class ReverseBytewiseComparatorImpl : public BytewiseComparatorImpl {
  public:
   ReverseBytewiseComparatorImpl() { }
 
-  static const char* kClassName() {
+  const char* Name() const override {
     return "rocksdb.ReverseBytewiseComparator";
   }
-  const char* Name() const override { return kClassName(); }
 
   int Compare(const Slice& a, const Slice& b) const override {
     return -a.compare(b);
@@ -227,77 +220,4 @@ const Comparator* ReverseBytewiseComparator() {
   return &rbytewise;
 }
 
-#ifndef ROCKSDB_LITE
-static int RegisterBuiltinComparators(ObjectLibrary& library,
-                                      const std::string& /*arg*/) {
-  library.Register<const Comparator>(
-      BytewiseComparatorImpl::kClassName(),
-      [](const std::string& /*uri*/,
-         std::unique_ptr<const Comparator>* /*guard */,
-         std::string* /* errmsg */) { return BytewiseComparator(); });
-  library.Register<const Comparator>(
-      ReverseBytewiseComparatorImpl::kClassName(),
-      [](const std::string& /*uri*/,
-         std::unique_ptr<const Comparator>* /*guard */,
-         std::string* /* errmsg */) { return ReverseBytewiseComparator(); });
-  return 2;
-}
-#endif  // ROCKSDB_LITE
-
-Status Comparator::CreateFromString(const ConfigOptions& config_options,
-                                    const std::string& value,
-                                    const Comparator** result) {
-#ifndef ROCKSDB_LITE
-  static std::once_flag once;
-  std::call_once(once, [&]() {
-    RegisterBuiltinComparators(*(ObjectLibrary::Default().get()), "");
-  });
-#endif  // ROCKSDB_LITE
-  std::string id;
-  std::unordered_map<std::string, std::string> opt_map;
-  Status status =
-      ConfigurableHelper::GetOptionsMap(value, *result, &id, &opt_map);
-  if (!status.ok()) {  // GetOptionsMap failed
-    return status;
-  }
-  std::string curr_opts;
-#ifndef ROCKSDB_LITE
-  if (*result != nullptr && (*result)->GetId() == id) {
-    // Try to get the existing options, ignoring any errors
-    ConfigOptions embedded = config_options;
-    embedded.delimiter = ";";
-    (*result)->GetOptionString(embedded, &curr_opts).PermitUncheckedError();
-  }
-#endif
-  if (id == BytewiseComparatorImpl::kClassName()) {
-    *result = BytewiseComparator();
-  } else if (id == ReverseBytewiseComparatorImpl::kClassName()) {
-    *result = ReverseBytewiseComparator();
-  } else if (value.empty()) {
-    // No Id and no options.  Clear the object
-    *result = nullptr;
-    return Status::OK();
-  } else if (id.empty()) {  // We have no Id but have options.  Not good
-    return Status::NotSupported("Cannot reset object ", id);
-  } else {
-#ifndef ROCKSDB_LITE
-    status = config_options.registry->NewStaticObject(id, result);
-#else
-    status = Status::NotSupported("Cannot load object in LITE mode ", id);
-#endif  // ROCKSDB_LITE
-    if (!status.ok()) {
-      if (config_options.ignore_unsupported_options &&
-          status.IsNotSupported()) {
-        return Status::OK();
-      } else {
-        return status;
-      }
-    } else if (!curr_opts.empty() || !opt_map.empty()) {
-      Comparator* comparator = const_cast<Comparator*>(*result);
-      status = ConfigurableHelper::ConfigureNewObject(
-          config_options, comparator, id, curr_opts, opt_map);
-    }
-  }
-  return status;
-}
 }  // namespace ROCKSDB_NAMESPACE