Remove deprecated SstFileWriter::Add() and skip_filters parameter (#14352)

Summary:
**Context/Summary:**
Remove `SstFileWriter::Add()` (deprecated in favor of `Put()`) and the `skip_filters` parameter from `SstFileWriter` constructors (deprecated in favor of setting `BlockBasedTableOptions::filter_policy` to `nullptr`).

Both APIs have zero active callers. The `skip_filters` field is also removed from `TableBuilderOptions` (write-side only; the read-side `TableReaderOptions::skip_filters` is unchanged).

Pull Request resolved: #14352

Test Plan: make check

Reviewed By: xingbowang

Differential Revision: D93812389

Pulled By: hx235

fbshipit-source-id: 236b36a6e664758ab5ad90e606bc195d0a6de70f

Author: hx235

db/external_sst_file_test.cc

@@ -2569,14 +2569,19 @@ TEST_F(ExternalSSTFileTest, SkipBloomFilter) {
         options.statistics->getTickerCount(Tickers::BLOCK_CACHE_FILTER_ADD), 1);
   }
 
-  // Create external SST file but skip bloom filters
+  // Create external SST file but skip bloom filters by using options
+  // with no filter policy
   options.statistics = ROCKSDB_NAMESPACE::CreateDBStatistics();
   DestroyAndReopen(options);
   {
     std::string file_path = sst_files_dir_ + "sst_with_no_bloom.sst";
-    SstFileWriter sst_file_writer(EnvOptions(), options, nullptr, true,
-                                  Env::IOPriority::IO_TOTAL,
-                                  true /* skip_filters */);
+    // Use options with no filter policy to skip bloom filters
+    Options no_filter_options = options;
+    BlockBasedTableOptions no_filter_table_options = table_options;
+    no_filter_table_options.filter_policy.reset();
+    no_filter_options.table_factory.reset(
+        NewBlockBasedTableFactory(no_filter_table_options));
+    SstFileWriter sst_file_writer(EnvOptions(), no_filter_options);
     ASSERT_OK(sst_file_writer.Open(file_path));
     ASSERT_OK(sst_file_writer.Put("Key1", "Value1"));
     ASSERT_OK(sst_file_writer.Finish());

include/rocksdb/sst_file_writer.h

@@ -82,37 +82,26 @@ class SstFileWriter {
   // hint that this file pages is not needed every time we write 1MB to the
   // file. To use the rate limiter an io_priority smaller than IO_TOTAL can be
   // passed.
-  // The `skip_filters` option is DEPRECATED and could be removed in the
-  // future. Use `BlockBasedTableOptions::filter_policy` to control filter
-  // generation.
   SstFileWriter(const EnvOptions& env_options, const Options& options,
                 ColumnFamilyHandle* column_family = nullptr,
                 bool invalidate_page_cache = true,
-                Env::IOPriority io_priority = Env::IOPriority::IO_TOTAL,
-                bool skip_filters = false)
+                Env::IOPriority io_priority = Env::IOPriority::IO_TOTAL)
       : SstFileWriter(env_options, options, options.comparator, column_family,
-                      invalidate_page_cache, io_priority, skip_filters) {}
+                      invalidate_page_cache, io_priority) {}
 
   // Deprecated API
   SstFileWriter(const EnvOptions& env_options, const Options& options,
                 const Comparator* user_comparator,
                 ColumnFamilyHandle* column_family = nullptr,
                 bool invalidate_page_cache = true,
-                Env::IOPriority io_priority = Env::IOPriority::IO_TOTAL,
-                bool skip_filters = false);
+                Env::IOPriority io_priority = Env::IOPriority::IO_TOTAL);
 
   ~SstFileWriter();
 
   // Prepare SstFileWriter to write into file located at "file_path".
   Status Open(const std::string& file_path,
               Temperature temp = Temperature::kUnknown);
 
-  // Add a Put key with value to currently opened file (deprecated)
-  // REQUIRES: user_key is after any previously added point (Put/Merge/Delete)
-  //           key according to the comparator.
-  // REQUIRES: comparator is *not* timestamp-aware.
-  [[deprecated]] Status Add(const Slice& user_key, const Slice& value);
-
   // Add a Put key with value to currently opened file
   // REQUIRES: user_key is after any previously added point (Put/Merge/Delete)
   //           key according to the comparator.

table/block_based/block_based_table_builder.cc

@@ -1269,9 +1269,6 @@ struct BlockBasedTableBuilder::Rep {
       // Apply optimize_filters_for_hits setting here when applicable by
       // skipping filter generation
       filter_builder.reset();
-    } else if (tbo.skip_filters) {
-      // For SstFileWriter skip_filters
-      filter_builder.reset();
     } else if (!table_options.filter_policy) {
       // Null filter_policy -> no filter
       filter_builder.reset();

table/sst_file_writer.cc

@@ -30,7 +30,7 @@ const size_t kFadviseTrigger = 1024 * 1024;  // 1MB
 struct SstFileWriter::Rep {
   Rep(const EnvOptions& _env_options, const Options& options,
       Env::IOPriority _io_priority, const Comparator* _user_comparator,
-      ColumnFamilyHandle* _cfh, bool _invalidate_page_cache, bool _skip_filters,
+      ColumnFamilyHandle* _cfh, bool _invalidate_page_cache,
       std::string _db_session_id)
       : env_options(_env_options),
         ioptions(options),
@@ -39,7 +39,6 @@ struct SstFileWriter::Rep {
         internal_comparator(_user_comparator),
         cfh(_cfh),
         invalidate_page_cache(_invalidate_page_cache),
-        skip_filters(_skip_filters),
         db_session_id(_db_session_id),
         ts_sz(_user_comparator->timestamp_size()),
         strip_timestamp(ts_sz > 0 &&
@@ -67,7 +66,6 @@ struct SstFileWriter::Rep {
   // The size of the file during the last time we called Fadvise to remove
   // cached pages from page cache.
   uint64_t last_fadvise_size = 0;
-  bool skip_filters;
   std::string db_session_id;
   uint64_t next_file_number = 1;
   size_t ts_sz;
@@ -305,9 +303,9 @@ SstFileWriter::SstFileWriter(const EnvOptions& env_options,
                              const Comparator* user_comparator,
                              ColumnFamilyHandle* column_family,
                              bool invalidate_page_cache,
-                             Env::IOPriority io_priority, bool skip_filters)
+                             Env::IOPriority io_priority)
     : rep_(new Rep(env_options, options, io_priority, user_comparator,
-                   column_family, invalidate_page_cache, skip_filters,
+                   column_family, invalidate_page_cache,
                    DBImpl::GenerateDbSessionId(options.env))) {
   // SstFileWriter is used to create sst files that can be added to database
   // later. Therefore, no real db_id and db_session_id are associated with it.
@@ -403,9 +401,6 @@ Status SstFileWriter::Open(const std::string& file_path, Temperature temp) {
   // assign fake file numbers to each file (into table properties) and keep
   // the same session id for the life of the SstFileWriter.
   r->next_file_number++;
-  // XXX: when we can remove skip_filters from the SstFileWriter public API
-  // we can remove it from TableBuilderOptions.
-  table_builder_options.skip_filters = r->skip_filters;
   FileTypeSet tmp_set = r->ioptions.checksum_handoff_file_types;
   r->file_writer.reset(new WritableFileWriter(
       std::move(sst_file), file_path, r->env_options, r->ioptions.clock,
@@ -424,10 +419,6 @@ Status SstFileWriter::Open(const std::string& file_path, Temperature temp) {
   return s;
 }
 
-Status SstFileWriter::Add(const Slice& user_key, const Slice& value) {
-  return rep_->Add(user_key, value, ValueType::kTypeValue);
-}
-
 Status SstFileWriter::Put(const Slice& user_key, const Slice& value) {
   return rep_->Add(user_key, value, ValueType::kTypeValue);
 }

table/table_builder.h

@@ -164,10 +164,6 @@ struct TableBuilderOptions : public TablePropertiesCollectorFactory::Context {
   const TableFileCreationReason reason;
   // END for FilterBuildingContext
 
-  // XXX: only used by BlockBasedTableBuilder for SstFileWriter. If you
-  // want to skip filters, that should be (for example) null filter_policy
-  // in the table options of the ioptions.table_factory
-  bool skip_filters = false;
   const uint64_t cur_file_num;
 };
 

unreleased_history/public_api_changes/remove_sst_file_writer_deprecated.md

@@ -0,0 +1 @@
+Remove deprecated `SstFileWriter::Add()` method (use `Put()` instead) and the deprecated `skip_filters` parameter from `SstFileWriter` constructors (use `BlockBasedTableOptions::filter_policy` set to `nullptr` to skip filter generation instead).