Make InternalKeyComparator not configurable

Summary:
InternalKeyComparator is an internal class which is a simple wrapper of Comparator. facebook#8336 made Comparator customizeable. As a side effect, internal key comparator was made configurable too. This introduces overhead to this simple wrapper. For example, every InternalKeyComparator will have an std::vector attached to it, which consumes memory and possible allocation overhead too.
We remove InternalKeyComparator from being customizable by making InternalKeyComparator not a subclass of Comparator.

Test Plan: Run existing CI tests and make sure it doesn't fail

HISTORY.md

@@ -24,6 +24,7 @@
 * Fix a bug in which backup/checkpoint can include a WAL deleted by RocksDB.
 * Fix a bug where concurrent compactions might cause unnecessary further write stalling. In some cases, this might cause write rate to drop to minimum.
 * Fix a bug in Logger where if dbname and db_log_dir are on different filesystems, dbname creation would fail wrt to db_log_dir path returning an error and fails to open the DB.
+* Fix the regression introduce by https://github.com/facebook/rocksdb/pull/8336 which made InternalKeyComparator configurable as an unintended side effect.
 
 ## Behavior Change
 * In leveled compaction with dynamic levelling, level multiplier is not anymore adjusted due to oversized L0. Instead, compaction score is adjusted by increasing size level target by adding incoming bytes from upper levels. This would deprioritize compactions from upper levels if more data from L0 is coming. This is to fix some unnecessary full stalling due to drastic change of level targets, while not wasting write bandwidth for compaction while writes are overloaded.

db/compaction/clipping_iterator.h

@@ -7,6 +7,7 @@
 
 #include <cassert>
 
+#include "rocksdb/comparator.h"
 #include "table/internal_iterator.h"
 
 namespace ROCKSDB_NAMESPACE {
@@ -19,7 +20,7 @@ namespace ROCKSDB_NAMESPACE {
 class ClippingIterator : public InternalIterator {
  public:
   ClippingIterator(InternalIterator* iter, const Slice* start, const Slice* end,
-                   const Comparator* cmp)
+                   const CompareInterface* cmp)
       : iter_(iter), start_(start), end_(end), cmp_(cmp), valid_(false) {
     assert(iter_);
     assert(cmp_);
@@ -268,7 +269,7 @@ class ClippingIterator : public InternalIterator {
   InternalIterator* iter_;
   const Slice* start_;
   const Slice* end_;
-  const Comparator* cmp_;
+  const CompareInterface* cmp_;
   bool valid_;
 };
 

db/dbformat.h

@@ -235,7 +235,7 @@ class InternalKeyComparator
 #ifdef NDEBUG
     final
 #endif
-    : public Comparator {
+    : public CompareInterface {
  private:
   UserComparatorWrapper user_comparator_;
 
@@ -253,13 +253,13 @@ class InternalKeyComparator
       : Comparator(c->timestamp_size()), user_comparator_(c) {}
   virtual ~InternalKeyComparator() {}
 
-  virtual const char* Name() const override;
+  virtual const char* Name() const;
   virtual int Compare(const Slice& a, const Slice& b) const override;
   // Same as Compare except that it excludes the value type from comparison
   virtual int CompareKeySeq(const Slice& a, const Slice& b) const;
   virtual void FindShortestSeparator(std::string* start,
-                                     const Slice& limit) const override;
-  virtual void FindShortSuccessor(std::string* key) const override;
+                                     const Slice& limit) const;
+  virtual void FindShortSuccessor(std::string* key) const;
 
   const Comparator* user_comparator() const {
     return user_comparator_.user_comparator();
@@ -273,7 +273,7 @@ class InternalKeyComparator
   // value `kDisableGlobalSequenceNumber`.
   int Compare(const Slice& a, SequenceNumber a_global_seqno, const Slice& b,
               SequenceNumber b_global_seqno) const;
-  virtual const Comparator* GetRootComparator() const override {
+  virtual const Comparator* GetRootComparator() const {
     return user_comparator_.GetRootComparator();
   }
 };

db/forward_iterator.h

@@ -4,6 +4,7 @@
 //  (found in the LICENSE.Apache file in the root directory).
 #pragma once
 
+#include "rocksdb/comparator.h"
 #ifndef ROCKSDB_LITE
 
 #include <string>
@@ -28,14 +29,14 @@ struct FileMetaData;
 
 class MinIterComparator {
  public:
-  explicit MinIterComparator(const Comparator* comparator) :
-    comparator_(comparator) {}
+  explicit MinIterComparator(const CompareInterface* comparator)
+      : comparator_(comparator) {}
 
   bool operator()(InternalIterator* a, InternalIterator* b) {
     return comparator_->Compare(a->key(), b->key()) > 0;
   }
  private:
-  const Comparator* comparator_;
+  const CompareInterface* comparator_;
 };
 
 using MinIterHeap =

include/rocksdb/comparator.h

@@ -17,6 +17,20 @@ namespace ROCKSDB_NAMESPACE {
 
 class Slice;
 
+class CompareInterface {
+ public:
+  virtual ~CompareInterface() {}
+
+  // Three-way comparison.  Returns value:
+  //   < 0 iff "a" < "b",
+  //   == 0 iff "a" == "b",
+  //   > 0 iff "a" > "b"
+  // Note that Compare(a, b) also compares timestamp if timestamp size is
+  // non-zero. For the same user key with different timestamps, larger (newer)
+  // timestamp comes first.
+  virtual int Compare(const Slice& a, const Slice& b) const = 0;
+};
+
 // A Comparator object provides a total order across slices that are
 // used as keys in an sstable or a database.  A Comparator implementation
 // must be thread-safe since rocksdb may invoke its methods concurrently
@@ -25,7 +39,7 @@ class Slice;
 // Exceptions MUST NOT propagate out of overridden functions into RocksDB,
 // because RocksDB is not exception-safe. This could cause undefined behavior
 // including data loss, unreported corruption, deadlocks, and more.
-class Comparator : public Customizable {
+class Comparator : public Customizable, public CompareInterface {
  public:
   Comparator() : timestamp_size_(0) {}
 
@@ -47,24 +61,6 @@ class Comparator : public Customizable {
                                  const Comparator** comp);
   static const char* Type() { return "Comparator"; }
 
-  // Three-way comparison.  Returns value:
-  //   < 0 iff "a" < "b",
-  //   == 0 iff "a" == "b",
-  //   > 0 iff "a" > "b"
-  // Note that Compare(a, b) also compares timestamp if timestamp size is
-  // non-zero. For the same user key with different timestamps, larger (newer)
-  // timestamp comes first.
-  virtual int Compare(const Slice& a, const Slice& b) const = 0;
-
-  // Compares two slices for equality. The following invariant should always
-  // hold (and is the default implementation):
-  //   Equal(a, b) iff Compare(a, b) == 0
-  // Overwrite only if equality comparisons can be done more efficiently than
-  // three-way comparisons.
-  virtual bool Equal(const Slice& a, const Slice& b) const {
-    return Compare(a, b) == 0;
-  }
-
   // The name of the comparator.  Used to check for comparator
   // mismatches (i.e., a DB created with one comparator is
   // accessed using a different comparator.
@@ -77,6 +73,15 @@ class Comparator : public Customizable {
   // by any clients of this package.
   const char* Name() const override = 0;
 
+  // Compares two slices for equality. The following invariant should always
+  // hold (and is the default implementation):
+  //   Equal(a, b) iff Compare(a, b) == 0
+  // Overwrite only if equality comparisons can be done more efficiently than
+  // three-way comparisons.
+  virtual bool Equal(const Slice& a, const Slice& b) const {
+    return Compare(a, b) == 0;
+  }
+
   // Advanced functions: these are used to reduce the space requirements
   // for internal data structures like index blocks.
 
@@ -91,10 +96,6 @@ class Comparator : public Customizable {
   // i.e., an implementation of this method that does nothing is correct.
   virtual void FindShortSuccessor(std::string* key) const = 0;
 
-  // if it is a wrapped comparator, may return the root one.
-  // return itself it is not wrapped.
-  virtual const Comparator* GetRootComparator() const { return this; }
-
   // given two keys, determine if t is the successor of s
   // BUG: only return true if no other keys starting with `t` are ordered
   // before `t`. Otherwise, the auto_prefix_mode can omit entries within
@@ -111,6 +112,10 @@ class Comparator : public Customizable {
   // with the customized comparator.
   virtual bool CanKeysWithDifferentByteContentsBeEqual() const { return true; }
 
+  // if it is a wrapped comparator, may return the root one.
+  // return itself it is not wrapped.
+  virtual const Comparator* GetRootComparator() const { return this; }
+
   inline size_t timestamp_size() const { return timestamp_size_; }
 
   int CompareWithoutTimestamp(const Slice& a, const Slice& b) const {

table/internal_iterator.h

@@ -187,7 +187,7 @@ class InternalIteratorBase : public Cleanable {
   virtual void SetReadaheadState(ReadaheadFileInfo* /*readahead_file_info*/) {}
 
  protected:
-  void SeekForPrevImpl(const Slice& target, const Comparator* cmp) {
+  void SeekForPrevImpl(const Slice& target, const CompareInterface* cmp) {
     Seek(target);
     if (!Valid()) {
       SeekToLast();

table/merging_iterator.cc

@@ -386,7 +386,7 @@ void MergingIterator::SwitchToForward() {
       if (child.status() == Status::TryAgain()) {
         continue;
       }
-      if (child.Valid() && comparator_->Equal(target, child.key())) {
+      if (child.Valid() && comparator_->Compare(target, child.key()) == 0) {
         assert(child.status().ok());
         child.Next();
       }
@@ -397,7 +397,7 @@ void MergingIterator::SwitchToForward() {
   for (auto& child : children_) {
     if (child.status() == Status::TryAgain()) {
       child.Seek(target);
-      if (child.Valid() && comparator_->Equal(target, child.key())) {
+      if (child.Valid() && comparator_->Compare(target, child.key()) == 0) {
         assert(child.status().ok());
         child.Next();
       }
@@ -416,7 +416,7 @@ void MergingIterator::SwitchToBackward() {
     if (&child != current_) {
       child.SeekForPrev(target);
       TEST_SYNC_POINT_CALLBACK("MergeIterator::Prev:BeforePrev", &child);
-      if (child.Valid() && comparator_->Equal(target, child.key())) {
+      if (child.Valid() && comparator_->Compare(target, child.key()) == 0) {
         assert(child.status().ok());
         child.Prev();
       }

table/sst_file_writer.cc

@@ -100,7 +100,7 @@ struct SstFileWriter::Rep {
   }
 
   Status Add(const Slice& user_key, const Slice& value, ValueType value_type) {
-    if (internal_comparator.timestamp_size() != 0) {
+    if (internal_comparator.user_comparator()->timestamp_size() != 0) {
       return Status::InvalidArgument("Timestamp size mismatch");
     }
 
@@ -111,7 +111,8 @@ struct SstFileWriter::Rep {
              ValueType value_type) {
     const size_t timestamp_size = timestamp.size();
 
-    if (internal_comparator.timestamp_size() != timestamp_size) {
+    if (internal_comparator.user_comparator()->timestamp_size() !=
+        timestamp_size) {
       return Status::InvalidArgument("Timestamp size mismatch");
     }
 
@@ -131,7 +132,7 @@ struct SstFileWriter::Rep {
   }
 
   Status DeleteRange(const Slice& begin_key, const Slice& end_key) {
-    if (internal_comparator.timestamp_size() != 0) {
+    if (internal_comparator.user_comparator()->timestamp_size() != 0) {
       return Status::InvalidArgument("Timestamp size mismatch");
     }
 

util/vector_iterator.h

@@ -6,6 +6,7 @@
 #include <vector>
 
 #include "db/dbformat.h"
+#include "rocksdb/comparator.h"
 #include "rocksdb/iterator.h"
 #include "rocksdb/slice.h"
 #include "table/internal_iterator.h"
@@ -16,7 +17,7 @@ namespace ROCKSDB_NAMESPACE {
 class VectorIterator : public InternalIterator {
  public:
   VectorIterator(std::vector<std::string> keys, std::vector<std::string> values,
-                 const Comparator* icmp = nullptr)
+                 const CompareInterface* icmp = nullptr)
       : keys_(std::move(keys)),
         values_(std::move(values)),
         current_(keys_.size()),
@@ -90,7 +91,7 @@ class VectorIterator : public InternalIterator {
 
  private:
   struct IndexedKeyComparator {
-    IndexedKeyComparator(const Comparator* c,
+    IndexedKeyComparator(const CompareInterface* c,
                          const std::vector<std::string>* ks)
         : cmp(c), keys(ks) {}
 
@@ -106,7 +107,7 @@ class VectorIterator : public InternalIterator {
       return cmp->Compare(a, (*keys)[b]) < 0;
     }
 
-    const Comparator* cmp;
+    const CompareInterface* cmp;
     const std::vector<std::string>* keys;
   };