fix a false positive case of parsing table factory from options file (#10094)

Summary:
During options file parsing, reset table factory before attempting to parse it
from string. This avoids mistakenly treating the default table factory as a
newly created one.

Signed-off-by: tabokie <xy.tao@outlook.com>
Pull Request resolved: https://github.com/facebook/rocksdb/pull/10094

Reviewed By: akankshamahajan15

Differential Revision: D36945378

Pulled By: ajkr

fbshipit-source-id: 94b2604e5e87682063b4b78f6370f3e8f101dc44

diff --git a/options/options_parser.cc b/options/options_parser.cc
index 40ae47487d2368dbd3b5800427cc5b3b2d7daffd..562a7b214b137a51b5cbb294762f6e8fc52f93e3 100644 (file)
--- a/options/options_parser.cc
+++ b/options/options_parser.cc
@@ -453,12 +453,13 @@ Status RocksDBOptionsParser::EndSection(
           section_arg);
     }
     // Ignore error as table factory deserialization is optional
+    cf_opt->table_factory.reset();
     s = TableFactory::CreateFromString(
         config_options,
         section_title.substr(
             opt_section_titles[kOptionSectionTableOptions].size()),
         &(cf_opt->table_factory));
-    if (s.ok()) {
+    if (s.ok() && cf_opt->table_factory != nullptr) {
       s = cf_opt->table_factory->ConfigureFromMap(config_options, opt_map);
       // Translate any errors (NotFound, NotSupported, to InvalidArgument
       if (s.ok() || s.IsInvalidArgument()) {

diff --git a/options/options_test.cc b/options/options_test.cc
index 8b0244ff22f265661019db5c3904e0ab1afd2b88..1992e39a54ba4c0bfccd25fdefb71f62860deb18 100644 (file)
--- a/options/options_test.cc
+++ b/options/options_test.cc
@@ -47,6 +47,22 @@ namespace ROCKSDB_NAMESPACE {
 
 class OptionsTest : public testing::Test {};
 
+class UnregisteredTableFactory : public TableFactory {
+ public:
+  UnregisteredTableFactory() {}
+  const char* Name() const override { return "Unregistered"; }
+  using TableFactory::NewTableReader;
+  Status NewTableReader(const ReadOptions&, const TableReaderOptions&,
+                        std::unique_ptr<RandomAccessFileReader>&&, uint64_t,
+                        std::unique_ptr<TableReader>*, bool) const override {
+    return Status::NotSupported();
+  }
+  TableBuilder* NewTableBuilder(const TableBuilderOptions&,
+                                WritableFileWriter*) const override {
+    return nullptr;
+  }
+};
+
 #ifndef ROCKSDB_LITE  // GetOptionsFromMap is not supported in ROCKSDB_LITE
 TEST_F(OptionsTest, GetOptionsFromMapTest) {
   std::unordered_map<std::string, std::string> cf_options_map = {
@@ -3662,6 +3678,10 @@ TEST_F(OptionsParserTest, DumpAndParse) {
       cf_opt.table_factory.reset(test::RandomTableFactory(&rnd, c));
     } else if (c == 4) {
       cf_opt.table_factory.reset(NewBlockBasedTableFactory(special_bbto));
+    } else if (c == 5) {
+      // A table factory that doesn't support deserialization should be
+      // supported.
+      cf_opt.table_factory.reset(new UnregisteredTableFactory());
     }
     base_cf_opts.emplace_back(cf_opt);
   }