From 838676c17bb34430f6024156178d04adc4d7be65 Mon Sep 17 00:00:00 2001 From: Islam AbdelRahman Date: Fri, 6 Nov 2015 16:49:38 -0800 Subject: [PATCH] Revert "Adding new table properties" Summary: Reverting https://reviews.facebook.net/D34269 for now after I landed it a flaky test started continuously failing, I am almost sure this patch is not related to the test but I will revert it until I figure out why it's failing Test Plan: make check Reviewers: kradhakrishnan Subscribers: dhruba Differential Revision: https://reviews.facebook.net/D50385 --- include/rocksdb/table_properties.h | 13 ----- table/block_based_table_builder.cc | 18 ------- table/meta_blocks.cc | 22 +------- table/table_properties.cc | 19 ------- table/table_test.cc | 87 ------------------------------ 5 files changed, 2 insertions(+), 157 deletions(-) diff --git a/include/rocksdb/table_properties.h b/include/rocksdb/table_properties.h index 160458700..73a825de8 100644 --- a/include/rocksdb/table_properties.h +++ b/include/rocksdb/table_properties.h @@ -53,16 +53,6 @@ struct TableProperties { // The name of the filter policy used in this table. // If no filter policy is used, `filter_policy_name` will be an empty string. std::string filter_policy_name; - // The name of the comparator used in this table. - std::string comparator_name; - // The name of the merge operator used in this table. - // If no merge operator is used, `merge_operator_name` will be - // an empty string. - std::string merge_operator_name; - // The names of the property collectors factories used in this table - // separated by commas - // {collector_name[1]},{collector_name[2]},{collector_name[3]} .. - std::string property_collectors_names; // user collected properties UserCollectedProperties user_collected_properties; @@ -90,9 +80,6 @@ struct TablePropertiesNames { static const std::string kFormatVersion; static const std::string kFixedKeyLen; static const std::string kFilterPolicy; - static const std::string kComparator; - static const std::string kMergeOperator; - static const std::string kPropertyCollectors; }; extern const std::string kPropertiesBlock; diff --git a/table/block_based_table_builder.cc b/table/block_based_table_builder.cc index ebccb596a..319235fbe 100644 --- a/table/block_based_table_builder.cc +++ b/table/block_based_table_builder.cc @@ -26,7 +26,6 @@ #include "rocksdb/env.h" #include "rocksdb/filter_policy.h" #include "rocksdb/flush_block_policy.h" -#include "rocksdb/merge_operator.h" #include "rocksdb/table.h" #include "table/block.h" @@ -789,23 +788,6 @@ Status BlockBasedTableBuilder::Finish() { r->table_options.filter_policy->Name() : ""; r->props.index_size = r->index_builder->EstimatedSize() + kBlockTrailerSize; - r->props.comparator_name = r->ioptions.comparator != nullptr - ? r->ioptions.comparator->Name() - : "N/A"; - r->props.merge_operator_name = r->ioptions.merge_operator != nullptr - ? r->ioptions.merge_operator->Name() - : "N/A"; - - std::string property_collectors_names = ""; - for (uint32_t i = 0; - i < r->ioptions.table_properties_collector_factories.size(); ++i) { - if (i != 0) { - property_collectors_names += ","; - } - property_collectors_names += - r->ioptions.table_properties_collector_factories[i]->Name(); - } - r->props.property_collectors_names = property_collectors_names; // Add basic properties property_block_builder.AddTableProperty(r->props); diff --git a/table/meta_blocks.cc b/table/meta_blocks.cc index c5f114290..505dbacd0 100644 --- a/table/meta_blocks.cc +++ b/table/meta_blocks.cc @@ -71,20 +71,8 @@ void PropertyBlockBuilder::AddTableProperty(const TableProperties& props) { Add(TablePropertiesNames::kFixedKeyLen, props.fixed_key_len); if (!props.filter_policy_name.empty()) { - Add(TablePropertiesNames::kFilterPolicy, props.filter_policy_name); - } - - if (!props.comparator_name.empty()) { - Add(TablePropertiesNames::kComparator, props.comparator_name); - } - - if (!props.merge_operator_name.empty()) { - Add(TablePropertiesNames::kMergeOperator, props.merge_operator_name); - } - - if (!props.property_collectors_names.empty()) { - Add(TablePropertiesNames::kPropertyCollectors, - props.property_collectors_names); + Add(TablePropertiesNames::kFilterPolicy, + props.filter_policy_name); } } @@ -215,12 +203,6 @@ Status ReadProperties(const Slice& handle_value, RandomAccessFileReader* file, *(pos->second) = val; } else if (key == TablePropertiesNames::kFilterPolicy) { new_table_properties->filter_policy_name = raw_val.ToString(); - } else if (key == TablePropertiesNames::kComparator) { - new_table_properties->comparator_name = raw_val.ToString(); - } else if (key == TablePropertiesNames::kMergeOperator) { - new_table_properties->merge_operator_name = raw_val.ToString(); - } else if (key == TablePropertiesNames::kPropertyCollectors) { - new_table_properties->property_collectors_names = raw_val.ToString(); } else { // handle user-collected properties new_table_properties->user_collected_properties.insert( diff --git a/table/table_properties.cc b/table/table_properties.cc index 8a29c03be..7a51779fe 100644 --- a/table/table_properties.cc +++ b/table/table_properties.cc @@ -75,20 +75,6 @@ std::string TableProperties::ToString( filter_policy_name.empty() ? std::string("N/A") : filter_policy_name, prop_delim, kv_delim); - AppendProperty(result, "comparator name", - comparator_name.empty() ? std::string("N/A") : comparator_name, - prop_delim, kv_delim); - - AppendProperty( - result, "merge operator name", - merge_operator_name.empty() ? std::string("N/A") : merge_operator_name, - prop_delim, kv_delim); - - AppendProperty(result, "property collectors names", - property_collectors_names.empty() ? std::string("N/A") - : property_collectors_names, - prop_delim, kv_delim); - return result; } @@ -122,11 +108,6 @@ const std::string TablePropertiesNames::kFormatVersion = "rocksdb.format.version"; const std::string TablePropertiesNames::kFixedKeyLen = "rocksdb.fixed.key.length"; -const std::string TablePropertiesNames::kComparator = "rocksdb.comparator"; -const std::string TablePropertiesNames::kMergeOperator = - "rocksdb.merge.operator"; -const std::string TablePropertiesNames::kPropertyCollectors = - "rocksdb.property.collectors"; extern const std::string kPropertiesBlock = "rocksdb.properties"; // Old property block name for backward compatibility diff --git a/table/table_test.cc b/table/table_test.cc index 4f079a3b2..58607bbb2 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -48,8 +48,6 @@ #include "util/testharness.h" #include "util/testutil.h" -#include "utilities/merge_operators.h" - namespace rocksdb { extern const uint64_t kLegacyBlockBasedTableMagicNumber; @@ -59,40 +57,6 @@ extern const uint64_t kPlainTableMagicNumber; namespace { -// DummyPropertiesCollector used to test BlockBasedTableProperties -class DummyPropertiesCollector : public TablePropertiesCollector { - public: - const char* Name() const { return ""; } - - Status Finish(UserCollectedProperties* properties) { return Status::OK(); } - - Status Add(const Slice& user_key, const Slice& value) { return Status::OK(); } - - virtual UserCollectedProperties GetReadableProperties() const { - return UserCollectedProperties{}; - } -}; - -class DummyPropertiesCollectorFactory1 - : public TablePropertiesCollectorFactory { - public: - virtual TablePropertiesCollector* CreateTablePropertiesCollector( - TablePropertiesCollectorFactory::Context context) { - return new DummyPropertiesCollector(); - } - const char* Name() const { return "DummyPropertiesCollector1"; } -}; - -class DummyPropertiesCollectorFactory2 - : public TablePropertiesCollectorFactory { - public: - virtual TablePropertiesCollector* CreateTablePropertiesCollector( - TablePropertiesCollectorFactory::Context context) { - return new DummyPropertiesCollector(); - } - const char* Name() const { return "DummyPropertiesCollector2"; } -}; - // Return reverse of "key". // Used to test non-lexicographic comparators. std::string Reverse(const Slice& key) { @@ -1054,57 +1018,6 @@ TEST_F(BlockBasedTableTest, BasicBlockBasedTableProperties) { ASSERT_EQ(content.size() + kBlockTrailerSize, props.data_size); } -TEST_F(BlockBasedTableTest, BlockBasedTableProperties2) { - TableConstructor c(&reverse_key_comparator); - std::vector keys; - stl_wrappers::KVMap kvmap; - - { - Options options; - BlockBasedTableOptions table_options; - options.table_factory.reset(NewBlockBasedTableFactory(table_options)); - - const ImmutableCFOptions ioptions(options); - c.Finish(options, ioptions, table_options, - GetPlainInternalComparator(options.comparator), &keys, &kvmap); - - auto& props = *c.GetTableReader()->GetTableProperties(); - - // Default comparator - ASSERT_EQ("leveldb.BytewiseComparator", props.comparator_name); - // No merge operator - ASSERT_EQ("N/A", props.merge_operator_name); - // No property collectors - ASSERT_EQ("", props.property_collectors_names); - // No filter policy is used - ASSERT_EQ("", props.filter_policy_name); - } - - { - Options options; - BlockBasedTableOptions table_options; - options.table_factory.reset(NewBlockBasedTableFactory(table_options)); - options.comparator = &reverse_key_comparator; - options.merge_operator = MergeOperators::CreateUInt64AddOperator(); - options.table_properties_collector_factories.emplace_back( - new DummyPropertiesCollectorFactory1()); - options.table_properties_collector_factories.emplace_back( - new DummyPropertiesCollectorFactory2()); - - const ImmutableCFOptions ioptions(options); - c.Finish(options, ioptions, table_options, - GetPlainInternalComparator(options.comparator), &keys, &kvmap); - - auto& props = *c.GetTableReader()->GetTableProperties(); - - ASSERT_EQ("rocksdb.ReverseBytewiseComparator", props.comparator_name); - ASSERT_EQ("UInt64AddOperator", props.merge_operator_name); - ASSERT_EQ("DummyPropertiesCollector1,DummyPropertiesCollector2", - props.property_collectors_names); - ASSERT_EQ("", props.filter_policy_name); // no filter policy is used - } -} - TEST_F(BlockBasedTableTest, FilterPolicyNameProperties) { TableConstructor c(BytewiseComparator(), true); c.Add("a1", "val1"); -- GitLab