Tests for filter compatibility #9773
Conversation
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: This change adds two unit tests that would each catch the regression fixed in facebook#9736 * TableMetaIndexKeys - detects any churn in metaindex block keys generated by SST files using standard db_test_util configurations. * BloomFilterCompatibility - this detects if any common built-in FilterPolicy configurations fail to read filters generated by another. (The regression bug caused NewRibbonFilterPolicy not to read filters from NewBloomFilterPolicy and vice-versa.) This replaces some previous tests that didn't really appear to be testing much of anything except basic data correctness, which doesn't tell you a filter is being used. Light refactoring in meta_blocks.cc/h to support inspecting metaindex keys. Test Plan: this is the test
pdillinger
force-pushed
the
bloom_compat_tests
branch
from
307aeaa
to
bce4bce
Compare
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| table_options.filter_policy.reset(NewBloomFilterPolicy(10, false)); | ||
| options.table_factory.reset(NewBlockBasedTableFactory(table_options)); | ||
| ReopenWithColumnFamilies({"default", "pikachu"}, options); | ||
| namespace { |
There was a problem hiding this comment.
Sorry that GitHub makes the diff on this file look super complicated. It's just complete replacement of some tests.
db/db_bloom_filter_test.cc
Outdated
| std::vector<CompatibilityConfig> kCompatibilityConfigs = { | ||
| {Create(20, kDeprecatedBlock), false, test::kDefaultFormatVersion}, | ||
| {kCompatibilityBloomPolicy, false, test::kDefaultFormatVersion}, | ||
| {kCompatibilityBloomPolicy, true, test::kDefaultFormatVersion}, |
There was a problem hiding this comment.
Needs BlockBasedTableOptions::kTwoLevelIndexSearch, otherwise partition_filters gets reset silently.
table/meta_blocks.cc
Outdated
| @@ -463,11 +463,10 @@ Status FindMetaBlock(InternalIterator* meta_index_iter, | |||
| } | |||
| } | |||
|
|
|||
| Status FindMetaBlockInFile(RandomAccessFileReader* file, uint64_t file_size, | |||
| Status ReadMetaBlockInFile(RandomAccessFileReader* file, uint64_t file_size, | |||
There was a problem hiding this comment.
Should it be ReadMetaIndexBlockInFile()? IIUC this is a distinct block from what FindMetaBlockInFile() and ReadMetaBlock() could return.
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: This change adds two unit tests that would each catch the
regression fixed in #9736
generated by SST files using standard db_test_util configurations.
FilterPolicy configurations fail to read filters generated by another.
(The regression bug caused NewRibbonFilterPolicy not to read filters
from NewBloomFilterPolicy and vice-versa.) This replaces some previous
tests that didn't really appear to be testing much of anything except
basic data correctness, which doesn't tell you a filter is being used.
Light refactoring in meta_blocks.cc/h to support inspecting metaindex
keys.
Test Plan: this is the test. Verified that 7.0.2 fails both tests and 7.0.3 passes.
With backporting for intentional API changes in 7.0, 6.29 also passes.