Removing NewTotalOrderPlainTableFactory

Summary:
Seems like NewTotalOrderPlainTableFactory is useless and is semantically incorrect.
Total order mode indicator is prefix_extractor == nullptr,
but NewTotalOrderPlainTableFactory doesn't set it to be nullptr. That's why some tests
in plain_table_db_tests is incorrect.

Test Plan: make all check

Reviewers: sdong

Reviewed By: sdong

Subscribers: leveldb

Differential Revision: https://reviews.facebook.net/D19587

HISTORY.md

@@ -1,6 +1,7 @@
 # Rocksdb Change Log
 
 ## Unreleased
+Removed NewTotalOrderPlainTableFactory because it was useless and was implemented semantically incorrect.
 
 ### New Features
 * HashLinklist reduces performance outlier caused by skewed bucket by switching data in the bucket from linked list to skip list. Add parameter threshold_use_skiplist in NewHashLinkListRepFactory().

db/plain_table_db_test.cc

@@ -262,16 +262,14 @@ TEST(PlainTableDBTest, Flush) {
     for (EncodingType encoding_type : {kPlain, kPrefix}) {
     for (int bloom_bits = 0; bloom_bits <= 117; bloom_bits += 117) {
       for (int total_order = 0; total_order <= 1; total_order++) {
-        if (encoding_type == kPrefix && total_order == 1) {
-          continue;
-        }
         Options options = CurrentOptions();
         options.create_if_missing = true;
         // Set only one bucket to force bucket conflict.
         // Test index interval for the same prefix to be 1, 2 and 4
         if (total_order) {
-          options.table_factory.reset(NewTotalOrderPlainTableFactory(
-              0, bloom_bits, 2, huge_page_tlb_size));
+          options.prefix_extractor.reset();
+          options.table_factory.reset(NewPlainTableFactory(
+              0, bloom_bits, 0, 2, huge_page_tlb_size, encoding_type));
         } else {
           options.table_factory.reset(NewPlainTableFactory(
               0, bloom_bits, 0.75, 16, huge_page_tlb_size, encoding_type));
@@ -290,8 +288,8 @@ TEST(PlainTableDBTest, Flush) {
         auto tp = row->second;
         ASSERT_EQ(total_order ? "4" : "12", (tp->user_collected_properties).at(
                                                 "plain_table_hash_table_size"));
-        ASSERT_EQ(total_order ? "9" : "0", (tp->user_collected_properties).at(
-                                               "plain_table_sub_index_size"));
+        ASSERT_EQ("0", (tp->user_collected_properties)
+                           .at("plain_table_sub_index_size"));
 
         ASSERT_EQ("v3", Get("1000000000000foo"));
         ASSERT_EQ("v2", Get("0000000000000bar"));
@@ -487,7 +485,7 @@ std::string MakeLongKey(size_t length, char c) {
 
 TEST(PlainTableDBTest, IteratorLargeKeys) {
   Options options = CurrentOptions();
-  options.table_factory.reset(NewTotalOrderPlainTableFactory(0, 0, 16, 0));
+  options.table_factory.reset(NewPlainTableFactory(0, 0, 0));
   options.create_if_missing = true;
   options.prefix_extractor.reset();
   DestroyAndReopen(&options);
@@ -668,7 +666,7 @@ TEST(PlainTableDBTest, HashBucketConflict) {
       // Set only one bucket to force bucket conflict.
       // Test index interval for the same prefix to be 1, 2 and 4
       options.table_factory.reset(
-          NewTotalOrderPlainTableFactory(16, 0, 2 ^ i, huge_page_tlb_size));
+          NewPlainTableFactory(16, 0, 0, 2 ^ i, huge_page_tlb_size));
       DestroyAndReopen(&options);
       ASSERT_OK(Put("5000000000000fo0", "v1"));
       ASSERT_OK(Put("5000000000000fo1", "v2"));
@@ -755,7 +753,7 @@ TEST(PlainTableDBTest, HashBucketConflictReverseSuffixComparator) {
       // Set only one bucket to force bucket conflict.
       // Test index interval for the same prefix to be 1, 2 and 4
       options.table_factory.reset(
-          NewTotalOrderPlainTableFactory(16, 0, 2 ^ i, huge_page_tlb_size));
+          NewPlainTableFactory(16, 0, 0, 2 ^ i, huge_page_tlb_size));
       DestroyAndReopen(&options);
       ASSERT_OK(Put("5000000000000fo0", "v1"));
       ASSERT_OK(Put("5000000000000fo1", "v2"));
@@ -835,7 +833,7 @@ TEST(PlainTableDBTest, NonExistingKeyToNonEmptyBucket) {
   options.create_if_missing = true;
   // Set only one bucket to force bucket conflict.
   // Test index interval for the same prefix to be 1, 2 and 4
-  options.table_factory.reset(NewTotalOrderPlainTableFactory(16, 0, 5));
+  options.table_factory.reset(NewPlainTableFactory(16, 0, 0, 5));
   DestroyAndReopen(&options);
   ASSERT_OK(Put("5000000000000fo0", "v1"));
   ASSERT_OK(Put("5000000000000fo1", "v2"));

include/rocksdb/table.h

@@ -155,35 +155,11 @@ struct PlainTablePropertyNames {
 //                 encoding types can co-exist in the same DB and can be read.
 
 const uint32_t kPlainTableVariableLength = 0;
-extern TableFactory* NewPlainTableFactory(uint32_t user_key_len =
-                                              kPlainTableVariableLength,
-                                          int bloom_bits_per_prefix = 10,
-                                          double hash_table_ratio = 0.75,
-                                          size_t index_sparseness = 16,
-                                          size_t huge_page_tlb_size = 0,
-                                          EncodingType encoding_type = kPlain);
-
-// -- Plain Table
-// This factory of plain table ignores Options.prefix_extractor and assumes no
-// hashable prefix available to the key structure. Lookup will be based on
-// binary search index only. Total order seek() can be issued.
-// @user_key_len: plain table has optimization for fix-sized keys, which can be
-//                specified via user_key_len.  Alternatively, you can pass
-//                `kPlainTableVariableLength` if your keys have variable
-//                lengths.
-// @bloom_bits_per_key: the number of bits used for bloom filer per key. You may
-//                  disable it by passing a zero.
-// @index_sparseness: need to build one index record for how many keys for
-//                    binary search.
-// @huge_page_tlb_size: if <=0, allocate hash indexes and blooms from malloc.
-//                      Otherwise from huge page TLB. The user needs to reserve
-//                      huge pages for it to be allocated, like:
-//                          sysctl -w vm.nr_hugepages=20
-//                      See linux doc Documentation/vm/hugetlbpage.txt
-extern TableFactory* NewTotalOrderPlainTableFactory(
+extern TableFactory* NewPlainTableFactory(
     uint32_t user_key_len = kPlainTableVariableLength,
-    int bloom_bits_per_key = 0, size_t index_sparseness = 16,
-    size_t huge_page_tlb_size = 0, bool full_scan_mode = false);
+    int bloom_bits_per_prefix = 10, double hash_table_ratio = 0.75,
+    size_t index_sparseness = 16, size_t huge_page_tlb_size = 0,
+    EncodingType encoding_type = kPlain, bool full_scan_mode = false);
 
 #endif  // ROCKSDB_LITE
 

table/plain_table_factory.cc

@@ -33,25 +33,13 @@ TableBuilder* PlainTableFactory::NewTableBuilder(
                                index_sparseness_);
 }
 
-extern TableFactory* NewPlainTableFactory(uint32_t user_key_len,
-                                          int bloom_bits_per_key,
-                                          double hash_table_ratio,
-                                          size_t index_sparseness,
-                                          size_t huge_page_tlb_size,
-                                          EncodingType encoding_type) {
-  return new PlainTableFactory(user_key_len, bloom_bits_per_key,
-                               hash_table_ratio, index_sparseness,
-                               huge_page_tlb_size, encoding_type);
-}
-
-extern TableFactory* NewTotalOrderPlainTableFactory(uint32_t user_key_len,
-                                                    int bloom_bits_per_key,
-                                                    size_t index_sparseness,
-                                                    size_t huge_page_tlb_size,
-                                                    bool full_scan_mode) {
-  return new PlainTableFactory(user_key_len, bloom_bits_per_key, 0,
-                               index_sparseness, huge_page_tlb_size, kPlain,
-                               full_scan_mode);
+extern TableFactory* NewPlainTableFactory(
+    uint32_t user_key_len, int bloom_bits_per_key, double hash_table_ratio,
+    size_t index_sparseness, size_t huge_page_tlb_size,
+    EncodingType encoding_type, bool full_scan_mode) {
+  return new PlainTableFactory(
+      user_key_len, bloom_bits_per_key, hash_table_ratio, index_sparseness,
+      huge_page_tlb_size, encoding_type, full_scan_mode);
 }
 
 const std::string PlainTablePropertyNames::kPrefixExtractorName =

table/table_test.cc

@@ -718,7 +718,8 @@ class Harness {
         only_support_prefix_seek_ = false;
         options_.prefix_extractor = nullptr;
         options_.allow_mmap_reads = true;
-        options_.table_factory.reset(NewTotalOrderPlainTableFactory());
+        options_.table_factory.reset(
+            NewPlainTableFactory(kPlainTableVariableLength, 0, 0));
         constructor_ = new TableConstructor(options_.comparator, true);
         internal_comparator_.reset(
             new InternalKeyComparator(options_.comparator));

tools/sst_dump.cc

@@ -157,8 +157,8 @@ Status SstFileReader::SetTableOptionsByMagicNumber(
   } else if (table_magic_number == kPlainTableMagicNumber ||
              table_magic_number == kLegacyPlainTableMagicNumber) {
     options_.allow_mmap_reads = true;
-    options_.table_factory.reset(NewTotalOrderPlainTableFactory(
-        kPlainTableVariableLength, 0, 1, 0, true));
+    options_.table_factory.reset(NewPlainTableFactory(
+        kPlainTableVariableLength, 0, 0, 1, 0, kPlain, true));
     fprintf(stdout, "Sst file format: plain table\n");
   } else {
     char error_msg_buffer[80];