From 33742c2a9f3706d6e776bcf810d58d8f16359b21 Mon Sep 17 00:00:00 2001 From: sdong Date: Tue, 1 Mar 2022 13:58:02 -0800 Subject: [PATCH] Remove BlockBasedTableOptions.hash_index_allow_collision (#9454) Summary: BlockBasedTableOptions.hash_index_allow_collision is already deprecated and has no effect. Delete it for preparing 7.0 release. Pull Request resolved: https://github.com/facebook/rocksdb/pull/9454 Test Plan: Run all existing tests. Reviewed By: ajkr Differential Revision: D33805827 fbshipit-source-id: ed8a436d1d083173ec6aef2a762ba02e1eefdc9d --- HISTORY.md | 3 +++ db/c.cc | 5 ----- db/db_bloom_filter_test.cc | 1 - examples/rocksdb_option_file_example.ini | 1 - include/rocksdb/c.h | 6 +++--- include/rocksdb/table.h | 5 ++--- options/options_settable_test.cc | 3 +-- options/options_test.cc | 6 ++---- table/block_based/block_based_table_factory.cc | 6 +----- table/block_based/block_based_table_reader.cc | 2 -- table/block_based/block_based_table_reader.h | 2 -- table/table_test.cc | 13 +++---------- test_util/testutil.cc | 1 - tools/db_bench_tool_test.cc | 1 - 14 files changed, 15 insertions(+), 40 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 80147d080..b3516ba44 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -7,6 +7,9 @@ ### Bug Fixes * * Fixed a data race on `versions_` between `DBImpl::ResumeImpl()` and threads waiting for recovery to complete (#9496) +### Public API changes +* Remove BlockBasedTableOptions.hash_index_allow_collision which already takes no effect. + ## 7.0.0 (02/20/2022) ### Bug Fixes * Fixed a major bug in which batched MultiGet could return old values for keys deleted by DeleteRange when memtable Bloom filter is enabled (memtable_prefix_bloom_size_ratio > 0). (The fix includes a substantial MultiGet performance improvement in the unusual case of both memtable_whole_key_filtering and prefix_extractor.) diff --git a/db/c.cc b/db/c.cc index c6f23adf2..0a181eecc 100644 --- a/db/c.cc +++ b/db/c.cc @@ -2297,11 +2297,6 @@ void rocksdb_block_based_options_set_data_block_hash_ratio( options->rep.data_block_hash_table_util_ratio = v; } -void rocksdb_block_based_options_set_hash_index_allow_collision( - rocksdb_block_based_table_options_t* options, unsigned char v) { - options->rep.hash_index_allow_collision = v; -} - void rocksdb_block_based_options_set_cache_index_and_filter_blocks( rocksdb_block_based_table_options_t* options, unsigned char v) { options->rep.cache_index_and_filter_blocks = v; diff --git a/db/db_bloom_filter_test.cc b/db/db_bloom_filter_test.cc index c2630d2e5..d4e576c6b 100644 --- a/db/db_bloom_filter_test.cc +++ b/db/db_bloom_filter_test.cc @@ -2163,7 +2163,6 @@ class BloomStatsTestWithParam options_.table_factory.reset(NewPlainTableFactory(table_options)); } else { BlockBasedTableOptions table_options; - table_options.hash_index_allow_collision = false; if (partition_filters_) { assert(bfp_impl_ != kDeprecatedBlock); table_options.partition_filters = partition_filters_; diff --git a/examples/rocksdb_option_file_example.ini b/examples/rocksdb_option_file_example.ini index 695200cad..351890e51 100644 --- a/examples/rocksdb_option_file_example.ini +++ b/examples/rocksdb_option_file_example.ini @@ -139,5 +139,4 @@ pin_l0_filter_and_index_blocks_in_cache=false pin_top_level_index_and_filter=false index_type=kBinarySearch - hash_index_allow_collision=true flush_block_policy_factory=FlushBlockBySizePolicyFactory diff --git a/include/rocksdb/c.h b/include/rocksdb/c.h index 9d9e999ca..aae4d3486 100644 --- a/include/rocksdb/c.h +++ b/include/rocksdb/c.h @@ -872,9 +872,9 @@ extern ROCKSDB_LIBRARY_API void rocksdb_block_based_options_set_data_block_index rocksdb_block_based_table_options_t*, int); // uses one of the above enums extern ROCKSDB_LIBRARY_API void rocksdb_block_based_options_set_data_block_hash_ratio( rocksdb_block_based_table_options_t* options, double v); -extern ROCKSDB_LIBRARY_API void -rocksdb_block_based_options_set_hash_index_allow_collision( - rocksdb_block_based_table_options_t*, unsigned char); +// rocksdb_block_based_options_set_hash_index_allow_collision() +// is removed since BlockBasedTableOptions.hash_index_allow_collision() +// is removed extern ROCKSDB_LIBRARY_API void rocksdb_block_based_options_set_cache_index_and_filter_blocks( rocksdb_block_based_table_options_t*, unsigned char); diff --git a/include/rocksdb/table.h b/include/rocksdb/table.h index e2daa2343..8782e94b0 100644 --- a/include/rocksdb/table.h +++ b/include/rocksdb/table.h @@ -227,9 +227,8 @@ struct BlockBasedTableOptions { // kDataBlockBinaryAndHash. double data_block_hash_table_util_ratio = 0.75; - // This option is now deprecated. No matter what value it is set to, - // it will behave as if hash_index_allow_collision=true. - bool hash_index_allow_collision = true; + // Option hash_index_allow_collision is now deleted. + // It will behave as if hash_index_allow_collision=true. // Use the specified checksum type. Newly created table files will be // protected with this checksum type. Old table files will still be readable, diff --git a/options/options_settable_test.cc b/options/options_settable_test.cc index 0c4d2c33b..bff7bbb7b 100644 --- a/options/options_settable_test.cc +++ b/options/options_settable_test.cc @@ -179,7 +179,7 @@ TEST_F(OptionsSettableTest, BlockBasedTableOptionsAllFieldsSettable) { "data_block_index_type=kDataBlockBinaryAndHash;" "index_shortening=kNoShortening;" "data_block_hash_table_util_ratio=0.75;" - "checksum=kxxHash;hash_index_allow_collision=1;no_block_cache=1;" + "checksum=kxxHash;no_block_cache=1;" "block_cache=1M;block_cache_compressed=1k;block_size=1024;" "block_size_deviation=8;block_restart_interval=4; " "metadata_block_size=1024;" @@ -190,7 +190,6 @@ TEST_F(OptionsSettableTest, BlockBasedTableOptionsAllFieldsSettable) { "construct_corruption=false;" "reserve_table_builder_memory=false;" "format_version=1;" - "hash_index_allow_collision=false;" "verify_compression=true;read_amp_bytes_per_bit=0;" "enable_index_compression=false;" "block_align=true;" diff --git a/options/options_test.cc b/options/options_test.cc index 565159c9c..00a40ff49 100644 --- a/options/options_test.cc +++ b/options/options_test.cc @@ -850,7 +850,7 @@ TEST_F(OptionsTest, GetBlockBasedTableOptionsFromString) { ASSERT_OK(GetBlockBasedTableOptionsFromString( config_options, table_opt, "cache_index_and_filter_blocks=1;index_type=kHashSearch;" - "checksum=kxxHash;hash_index_allow_collision=1;" + "checksum=kxxHash;" "block_cache=1M;block_cache_compressed=1k;block_size=1024;" "block_size_deviation=8;block_restart_interval=4;" "format_version=5;whole_key_filtering=1;" @@ -866,7 +866,6 @@ TEST_F(OptionsTest, GetBlockBasedTableOptionsFromString) { ASSERT_TRUE(new_opt.cache_index_and_filter_blocks); ASSERT_EQ(new_opt.index_type, BlockBasedTableOptions::kHashSearch); ASSERT_EQ(new_opt.checksum, ChecksumType::kxxHash); - ASSERT_TRUE(new_opt.hash_index_allow_collision); ASSERT_TRUE(new_opt.block_cache != nullptr); ASSERT_EQ(new_opt.block_cache->GetCapacity(), 1024UL*1024UL); ASSERT_TRUE(new_opt.block_cache_compressed != nullptr); @@ -2814,7 +2813,7 @@ TEST_F(OptionsOldApiTest, GetBlockBasedTableOptionsFromString) { ASSERT_OK(GetBlockBasedTableOptionsFromString( table_opt, "cache_index_and_filter_blocks=1;index_type=kHashSearch;" - "checksum=kxxHash;hash_index_allow_collision=1;no_block_cache=1;" + "checksum=kxxHash;no_block_cache=1;" "block_cache=1M;block_cache_compressed=1k;block_size=1024;" "block_size_deviation=8;block_restart_interval=4;" "format_version=5;whole_key_filtering=1;" @@ -2823,7 +2822,6 @@ TEST_F(OptionsOldApiTest, GetBlockBasedTableOptionsFromString) { ASSERT_TRUE(new_opt.cache_index_and_filter_blocks); ASSERT_EQ(new_opt.index_type, BlockBasedTableOptions::kHashSearch); ASSERT_EQ(new_opt.checksum, ChecksumType::kxxHash); - ASSERT_TRUE(new_opt.hash_index_allow_collision); ASSERT_TRUE(new_opt.no_block_cache); ASSERT_TRUE(new_opt.block_cache != nullptr); ASSERT_EQ(new_opt.block_cache->GetCapacity(), 1024UL*1024UL); diff --git a/table/block_based/block_based_table_factory.cc b/table/block_based/block_based_table_factory.cc index d69b296dc..5f13b245b 100644 --- a/table/block_based/block_based_table_factory.cc +++ b/table/block_based/block_based_table_factory.cc @@ -258,8 +258,7 @@ static std::unordered_map offsetof(struct BlockBasedTableOptions, index_type), &block_base_table_index_type_string_map)}, {"hash_index_allow_collision", - {offsetof(struct BlockBasedTableOptions, hash_index_allow_collision), - OptionType::kBoolean, OptionVerificationType::kNormal, + {0, OptionType::kBoolean, OptionVerificationType::kDeprecated, OptionTypeFlags::kNone}}, {"data_block_index_type", OptionTypeInfo::Enum( @@ -706,9 +705,6 @@ std::string BlockBasedTableFactory::GetPrintableOptions() const { snprintf(buffer, kBufferSize, " data_block_hash_table_util_ratio: %lf\n", table_options_.data_block_hash_table_util_ratio); ret.append(buffer); - snprintf(buffer, kBufferSize, " hash_index_allow_collision: %d\n", - table_options_.hash_index_allow_collision); - ret.append(buffer); snprintf(buffer, kBufferSize, " checksum: %d\n", table_options_.checksum); ret.append(buffer); snprintf(buffer, kBufferSize, " no_block_cache: %d\n", diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index 686f38ba1..3a1643846 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -621,8 +621,6 @@ Status BlockBasedTable::Open( file_size, level, immortal_table); rep->file = std::move(file); rep->footer = footer; - rep->hash_index_allow_collision = table_options.hash_index_allow_collision; - // We've successfully read the footer. We are ready to serve requests. // Better not mutate rep_ after the creation. eg. internal_prefix_transform // raw pointer will be used to create HashIndexReader, whose reset may diff --git a/table/block_based/block_based_table_reader.h b/table/block_based/block_based_table_reader.h index 1163449da..29736d326 100644 --- a/table/block_based/block_based_table_reader.h +++ b/table/block_based/block_based_table_reader.h @@ -546,7 +546,6 @@ struct BlockBasedTable::Rep { internal_comparator(_internal_comparator), filter_type(FilterType::kNoFilter), index_type(BlockBasedTableOptions::IndexType::kBinarySearch), - hash_index_allow_collision(false), whole_key_filtering(_table_opt.whole_key_filtering), prefix_filtering(true), global_seqno(kDisableGlobalSequenceNumber), @@ -583,7 +582,6 @@ struct BlockBasedTable::Rep { std::shared_ptr table_properties; BlockBasedTableOptions::IndexType index_type; - bool hash_index_allow_collision; bool whole_key_filtering; bool prefix_filtering; // TODO(kailiu) It is very ugly to use internal key in table, since table diff --git a/table/table_test.cc b/table/table_test.cc index b847cb70a..52a062286 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -2016,7 +2016,7 @@ TEST_P(BlockBasedTableTest, PrefetchTest) { TEST_P(BlockBasedTableTest, TotalOrderSeekOnHashIndex) { BlockBasedTableOptions table_options = GetBlockBasedTableOptions(); - for (int i = 0; i <= 5; ++i) { + for (int i = 0; i <= 4; ++i) { Options options; // Make each key/value an individual block table_options.block_size = 64; @@ -2033,25 +2033,18 @@ TEST_P(BlockBasedTableTest, TotalOrderSeekOnHashIndex) { options.prefix_extractor.reset(NewFixedPrefixTransform(4)); break; case 2: - // Hash search index with hash_index_allow_collision - table_options.index_type = BlockBasedTableOptions::kHashSearch; - table_options.hash_index_allow_collision = true; - options.table_factory.reset(new BlockBasedTableFactory(table_options)); - options.prefix_extractor.reset(NewFixedPrefixTransform(4)); - break; - case 3: // Hash search index with filter policy table_options.index_type = BlockBasedTableOptions::kHashSearch; table_options.filter_policy.reset(NewBloomFilterPolicy(10)); options.table_factory.reset(new BlockBasedTableFactory(table_options)); options.prefix_extractor.reset(NewFixedPrefixTransform(4)); break; - case 4: + case 3: // Two-level index table_options.index_type = BlockBasedTableOptions::kTwoLevelIndexSearch; options.table_factory.reset(new BlockBasedTableFactory(table_options)); break; - case 5: + case 4: // Binary search with first key table_options.index_type = BlockBasedTableOptions::kBinarySearchWithFirstKey; diff --git a/test_util/testutil.cc b/test_util/testutil.cc index 14b0c181a..cc95d8956 100644 --- a/test_util/testutil.cc +++ b/test_util/testutil.cc @@ -231,7 +231,6 @@ BlockBasedTableOptions RandomBlockBasedTableOptions(Random* rnd) { IndexType::kTwoLevelIndexSearch, IndexType::kBinarySearchWithFirstKey}}; opt.index_type = index_types[rnd->Uniform(static_cast(index_types.size()))]; - opt.hash_index_allow_collision = rnd->Uniform(2); opt.checksum = static_cast(rnd->Uniform(3)); opt.block_size = rnd->Uniform(10000000); opt.block_size_deviation = rnd->Uniform(100); diff --git a/tools/db_bench_tool_test.cc b/tools/db_bench_tool_test.cc index d86f58357..2b78a06d3 100644 --- a/tools/db_bench_tool_test.cc +++ b/tools/db_bench_tool_test.cc @@ -281,7 +281,6 @@ const std::string options_file_content = R"OPTIONS_FILE( skip_table_builder_flush=false cache_index_and_filter_blocks=false flush_block_policy_factory=FlushBlockBySizePolicyFactory - hash_index_allow_collision=true index_type=kBinarySearch whole_key_filtering=true checksum=kCRC32c