Sanitize block-based table index type and check prefix_extractor

Summary:
Respond to issue reported
https://www.facebook.com/groups/rocksdb.dev/permalink/651090261656158/
Change the Sanitize signature to take both DBOptions and CFOptions

Test Plan: unit test

Reviewers: sdong, yhchiang, igor

Reviewed By: igor

Subscribers: leveldb

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

db/db_impl.cc

@@ -282,12 +282,12 @@ DBOptions SanitizeOptions(const std::string& dbname, const DBOptions& src) {
 
 namespace {
 
-Status SanitizeDBOptionsByCFOptions(
+Status SanitizeOptionsByTable(
-    const DBOptions* db_opts,
+    const DBOptions& db_opts,
     const std::vector<ColumnFamilyDescriptor>& column_families) {
   Status s;
   for (auto cf : column_families) {
-    s = cf.options.table_factory->SanitizeDBOptions(db_opts);
+    s = cf.options.table_factory->SanitizeOptions(db_opts, cf.options);
     if (!s.ok()) {
       return s;
     }
@@ -4703,7 +4703,7 @@ Status DB::Open(const Options& options, const std::string& dbname, DB** dbptr) {
 Status DB::Open(const DBOptions& db_options, const std::string& dbname,
                 const std::vector<ColumnFamilyDescriptor>& column_families,
                 std::vector<ColumnFamilyHandle*>* handles, DB** dbptr) {
-  Status s = SanitizeDBOptionsByCFOptions(&db_options, column_families);
+  Status s = SanitizeOptionsByTable(db_options, column_families);
   if (!s.ok()) {
     return s;
   }

db/db_test.cc

@@ -8354,6 +8354,17 @@ TEST(DBTest, TableOptionsSanitizeTest) {
   options.prefix_extractor.reset(NewNoopTransform());
   Destroy(&options);
   ASSERT_TRUE(TryReopen(&options).IsNotSupported());
+
+  // Test for check of prefix_extractor when hash index is used for
+  // block-based table
+  BlockBasedTableOptions to;
+  to.index_type = BlockBasedTableOptions::kHashSearch;
+  options = Options();
+  options.create_if_missing = true;
+  options.table_factory.reset(NewBlockBasedTableFactory(to));
+  ASSERT_TRUE(TryReopen(&options).IsInvalidArgument());
+  options.prefix_extractor.reset(NewFixedPrefixTransform(1));
+  ASSERT_OK(TryReopen(&options));
 }
 
 TEST(DBTest, DBIteratorBoundTest) {

include/rocksdb/table.h

@@ -355,11 +355,13 @@ class TableFactory {
       WritableFile* file, const CompressionType compression_type,
       const CompressionOptions& compression_opts) const = 0;
 
-  // Sanitizes the specified DB Options.
+  // Sanitizes the specified DB Options and ColumnFamilyOptions.
   //
   // If the function cannot find a way to sanitize the input DB Options,
   // a non-ok Status will be returned.
-  virtual Status SanitizeDBOptions(const DBOptions* db_opts) const = 0;
+  virtual Status SanitizeOptions(
+      const DBOptions& db_opts,
+      const ColumnFamilyOptions& cf_opts) const = 0;
 
   // Return a string that contains printable format of table configurations.
   // RocksDB prints configurations at DB Open().

table/adaptive_table_factory.h

@@ -47,8 +47,9 @@ class AdaptiveTableFactory : public TableFactory {
       const CompressionOptions& compression_opts) const override;
 
   // Sanitizes the specified DB Options.
-  Status SanitizeDBOptions(const DBOptions* db_opts) const override {
+  Status SanitizeOptions(const DBOptions& db_opts,
-    if (db_opts->allow_mmap_reads == false) {
+                         const ColumnFamilyOptions& cf_opts) const override {
+    if (db_opts.allow_mmap_reads == false) {
       return Status::NotSupported(
           "AdaptiveTable with allow_mmap_reads == false is not supported.");
     }

table/block_based_table_factory.cc

@@ -63,6 +63,17 @@ TableBuilder* BlockBasedTableFactory::NewTableBuilder(
   return table_builder;
 }
 
+Status BlockBasedTableFactory::SanitizeOptions(
+    const DBOptions& db_opts,
+    const ColumnFamilyOptions& cf_opts) const {
+  if (table_options_.index_type == BlockBasedTableOptions::kHashSearch &&
+      cf_opts.prefix_extractor == nullptr) {
+    return Status::InvalidArgument("Hash index is specified for block-based "
+        "table, but prefix_extractor is not given");
+  }
+  return Status::OK();
+}
+
 std::string BlockBasedTableFactory::GetPrintableTableOptions() const {
   std::string ret;
   ret.reserve(20000);

table/block_based_table_factory.h

@@ -46,9 +46,8 @@ class BlockBasedTableFactory : public TableFactory {
       const CompressionOptions& compression_opts) const override;
 
   // Sanitizes the specified DB Options.
-  Status SanitizeDBOptions(const DBOptions* db_opts) const override {
+  Status SanitizeOptions(const DBOptions& db_opts,
-    return Status::OK();
+                         const ColumnFamilyOptions& cf_opts) const override;
-  }
 
   std::string GetPrintableTableOptions() const override;
 

table/cuckoo_table_factory.h

@@ -64,7 +64,8 @@ class CuckooTableFactory : public TableFactory {
       const CompressionType, const CompressionOptions&) const override;
 
   // Sanitizes the specified DB Options.
-  Status SanitizeDBOptions(const DBOptions* db_opts) const override {
+  Status SanitizeOptions(const DBOptions& db_opts,
+                         const ColumnFamilyOptions& cf_opts) const override {
     return Status::OK();
   }
 

table/plain_table_factory.h

@@ -170,8 +170,9 @@ class PlainTableFactory : public TableFactory {
   static const char kValueTypeSeqId0 = 0xFF;
 
   // Sanitizes the specified DB Options.
-  Status SanitizeDBOptions(const DBOptions* db_opts) const override {
+  Status SanitizeOptions(const DBOptions& db_opts,
-    if (db_opts->allow_mmap_reads == false) {
+                         const ColumnFamilyOptions& cf_opts) const override {
+    if (db_opts.allow_mmap_reads == false) {
       return Status::NotSupported(
           "PlainTable with allow_mmap_reads == false is not supported.");
     }