This is an automated email from the ASF dual-hosted git repository. alexey pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 5557c488e1c4264c20b764235b4830456f73ce3b Author: Alexey Serbin <[email protected]> AuthorDate: Tue Feb 15 13:43:53 2022 -0800 [schema] use operator==() instead of Equals() While looking at the code in src/kudu/common/schema.h in the scope of reviewing a prior changelist, I noticed that the comparison options for Schema::Equals() were used only in the test scenarios. I took the liberty and moved the Equals() method into the test code and replaced it with the operator== at call sites. This patch includes other minor cleanup, but overall the patch doesn't contain any functional changes. Change-Id: Ib0e3d75d216241efec49b64233198eafe52871cb Reviewed-on: http://gerrit.cloudera.org:8080/18237 Tested-by: Alexey Serbin <[email protected]> Reviewed-by: Abhishek Chennaka <[email protected]> Reviewed-by: Andrew Wong <[email protected]> --- src/kudu/client/client.cc | 8 +-- src/kudu/client/schema.cc | 3 +- src/kudu/common/generic_iterators.cc | 4 +- src/kudu/common/row.h | 6 +- src/kudu/common/schema-test.cc | 88 +++++++++++++++++--------- src/kudu/common/schema.h | 51 ++++++--------- src/kudu/integration-tests/alter_table-test.cc | 4 +- src/kudu/master/master-test.cc | 2 +- src/kudu/master/sys_catalog.cc | 2 +- src/kudu/tablet/compaction-test.cc | 2 +- src/kudu/tablet/tablet.cc | 2 +- src/kudu/tablet/tablet_metadata.cc | 2 +- src/kudu/tserver/tablet_service.cc | 4 +- 13 files changed, 98 insertions(+), 80 deletions(-) diff --git a/src/kudu/client/client.cc b/src/kudu/client/client.cc index e26a46c..4b22424 100644 --- a/src/kudu/client/client.cc +++ b/src/kudu/client/client.cc @@ -1515,13 +1515,13 @@ KuduTableAlterer* KuduTableAlterer::AddRangePartitionWithDimension( data_->status_ = Status::InvalidArgument("range partition bounds may not be null"); return this; } - if (!lower_bound->schema()->Equals(*upper_bound->schema())) { + if (*lower_bound->schema() != *upper_bound->schema()) { data_->status_ = Status::InvalidArgument("range partition bounds must have matching schemas"); return this; } if (data_->schema_ == nullptr) { data_->schema_ = lower_bound->schema(); - } else if (!lower_bound->schema()->Equals(*data_->schema_)) { + } else if (*lower_bound->schema() != *data_->schema_) { data_->status_ = Status::InvalidArgument("range partition bounds must have matching schemas"); return this; } @@ -1547,13 +1547,13 @@ KuduTableAlterer* KuduTableAlterer::DropRangePartition( data_->status_ = Status::InvalidArgument("range partition bounds may not be null"); return this; } - if (!lower_bound->schema()->Equals(*upper_bound->schema())) { + if (*lower_bound->schema() != *upper_bound->schema()) { data_->status_ = Status::InvalidArgument("range partition bounds must have matching schemas"); return this; } if (data_->schema_ == nullptr) { data_->schema_ = lower_bound->schema(); - } else if (!lower_bound->schema()->Equals(*data_->schema_)) { + } else if (*lower_bound->schema() != *data_->schema_) { data_->status_ = Status::InvalidArgument("range partition bounds must have matching schemas"); return this; } diff --git a/src/kudu/client/schema.cc b/src/kudu/client/schema.cc index 8c97e55..b53b5fe 100644 --- a/src/kudu/client/schema.cc +++ b/src/kudu/client/schema.cc @@ -878,8 +878,7 @@ Status KuduSchema::Reset(const vector<KuduColumnSchema>& columns, int key_column } bool KuduSchema::operator==(const KuduSchema& rhs) const { - return this == &rhs || - (schema_ && rhs.schema_ && schema_->Equals(*rhs.schema_)); + return this == &rhs || (schema_ && rhs.schema_ && (*schema_ == *rhs.schema_)); } bool KuduSchema::operator!=(const KuduSchema& rhs) const { diff --git a/src/kudu/common/generic_iterators.cc b/src/kudu/common/generic_iterators.cc index c2c3851..ad9fa17 100644 --- a/src/kudu/common/generic_iterators.cc +++ b/src/kudu/common/generic_iterators.cc @@ -616,7 +616,7 @@ Status MergeIterator::Init(ScanSpec *spec) { finished_iter_stats_by_col_.resize(schema_->num_columns()); #ifndef NDEBUG for (const auto& s : states_) { - if (!s.schema().Equals(*schema_)) { + if (s.schema() != *schema_) { return Status::InvalidArgument( Substitute("Schemas do not match: $0 vs. $1", schema_->ToString(), s.schema().ToString())); @@ -1008,7 +1008,7 @@ Status UnionIterator::Init(ScanSpec *spec) { finished_iter_stats_by_col_.resize(schema_->num_columns()); #ifndef NDEBUG for (const auto& i : iters_) { - if (!i.iter->schema().Equals(*schema_)) { + if (i.iter->schema() != *schema_) { return Status::InvalidArgument( Substitute("Schemas do not match: $0 vs. $1", schema_->ToString(), i.iter->schema().ToString())); diff --git a/src/kudu/common/row.h b/src/kudu/common/row.h index a2e3756..aa63241 100644 --- a/src/kudu/common/row.h +++ b/src/kudu/common/row.h @@ -144,7 +144,7 @@ class RowProjector { // The two Schema pointers must remain valid for the lifetime of this object. RowProjector(const Schema* base_schema, const Schema* projection) : base_schema_(base_schema), projection_(projection), - is_identity_(base_schema->Equals(*projection)) { + is_identity_(*base_schema == *projection) { } // Initialize the projection mapping with the specified base_schema and projection @@ -157,7 +157,7 @@ class RowProjector { projection_ = projection; base_cols_mapping_.clear(); projection_defaults_.clear(); - is_identity_ = base_schema->Equals(*projection); + is_identity_ = (*base_schema == *projection); return Init(); } @@ -275,7 +275,7 @@ class DeltaProjector { // of the object. DeltaProjector(const Schema* delta_schema, const Schema* projection) : delta_schema_(delta_schema), projection_(projection), - is_identity_(delta_schema->Equals(*projection)) { + is_identity_(*delta_schema == *projection) { } Status Init() { diff --git a/src/kudu/common/schema-test.cc b/src/kudu/common/schema-test.cc index 251402b..ae79cf8 100644 --- a/src/kudu/common/schema-test.cc +++ b/src/kudu/common/schema-test.cc @@ -21,7 +21,6 @@ #include <cstdint> #include <string> #include <tuple> // IWYU pragma: keep -#include <unordered_map> #include <utility> #include <vector> @@ -44,19 +43,52 @@ #include "kudu/util/test_macros.h" #include "kudu/util/test_util.h" -namespace kudu { -namespace tablet { - using std::string; -using std::unordered_map; using std::vector; using strings::Substitute; +namespace kudu { + +// Return true if the schemas have exactly the same set of columns +// and respective types. +bool EqualSchemas(const Schema& lhs, const Schema& rhs) { + if (lhs != rhs) { + return false; + } + + if (lhs.num_key_columns_ != rhs.num_key_columns_) { + return false; + } + if (lhs.num_columns() != rhs.num_columns()) { + return false; + } + for (size_t i = 0; i < rhs.num_columns(); ++i) { + if (!lhs.cols_[i].Equals(rhs.cols_[i])) { + return false; + } + } + + if (lhs.has_column_ids() != rhs.has_column_ids()) { + return false; + } + if (lhs.has_column_ids()) { + if (lhs.col_ids_ != rhs.col_ids_) { + return false; + } + if (lhs.max_col_id() != rhs.max_col_id()) { + return false; + } + } + + return true; +} + +namespace tablet { + // Copy a row and its referenced data into the given Arena. -static Status CopyRowToArena(const Slice &row, - const Schema &schema, - Arena *dst_arena, - ContiguousRow *copied) { +static Status CopyRowToArena(const Slice& row, + Arena* dst_arena, + ContiguousRow* copied) { Slice row_data; // Copy the direct row data to arena @@ -65,8 +97,7 @@ static Status CopyRowToArena(const Slice &row, } copied->Reset(row_data.mutable_data()); - RETURN_NOT_OK(RelocateIndirectDataToArena(copied, dst_arena)); - return Status::OK(); + return RelocateIndirectDataToArena(copied, dst_arena); } class TestSchema : public KuduTest {}; @@ -159,11 +190,10 @@ TEST_P(ParameterizedSchemaTest, TestCopyAndMove) { vector<ColumnSchema> cols = { col1, col2, col3 }; vector<ColumnId> ids = { ColumnId(0), ColumnId(1), ColumnId(2) }; - const int kNumKeyCols = 1; + constexpr int kNumKeyCols = 1; - Schema schema = GetParam() == INCLUDE_COL_IDS ? - Schema(cols, ids, kNumKeyCols) : - Schema(cols, kNumKeyCols); + const auto& schema = GetParam() == INCLUDE_COL_IDS + ? Schema(cols, ids, kNumKeyCols) : Schema(cols, kNumKeyCols); NO_FATALS(check_schema(schema)); @@ -172,7 +202,7 @@ TEST_P(ParameterizedSchemaTest, TestCopyAndMove) { { Schema copied_schema = schema; NO_FATALS(check_schema(copied_schema)); - ASSERT_TRUE(copied_schema.Equals(schema, Schema::COMPARE_ALL)); + ASSERT_TRUE(EqualSchemas(schema, copied_schema)); // Move-assign to 'moved_to_schema' from 'copied_schema' and then let // 'copied_schema' go out of scope to make sure none of the 'moved_schema' @@ -184,18 +214,18 @@ TEST_P(ParameterizedSchemaTest, TestCopyAndMove) { copied_schema.ToString(); // NOLINT(*) } NO_FATALS(check_schema(moved_schema)); - ASSERT_TRUE(moved_schema.Equals(schema, Schema::COMPARE_ALL)); + ASSERT_TRUE(EqualSchemas(schema, moved_schema)); // Check copy- and move-construction. { Schema copied_schema(schema); NO_FATALS(check_schema(copied_schema)); - ASSERT_TRUE(copied_schema.Equals(schema, Schema::COMPARE_ALL)); + ASSERT_TRUE(EqualSchemas(schema, copied_schema)); Schema moved_schema(std::move(copied_schema)); copied_schema.ToString(); // NOLINT(*) NO_FATALS(check_schema(moved_schema)); - ASSERT_TRUE(moved_schema.Equals(schema, Schema::COMPARE_ALL)); + ASSERT_TRUE(EqualSchemas(schema, moved_schema)); } } @@ -254,10 +284,10 @@ TEST_F(TestSchema, TestSchemaEqualsWithDecimal) { Schema schema_17_10({ col1, col_17_10 }, 1); Schema schema_17_9({ col1, col_17_9 }, 1); - EXPECT_TRUE(schema_18_10.Equals(schema_18_10)); - EXPECT_FALSE(schema_18_10.Equals(schema_18_9)); - EXPECT_FALSE(schema_18_10.Equals(schema_17_10)); - EXPECT_FALSE(schema_18_10.Equals(schema_17_9)); + EXPECT_EQ(schema_18_10, schema_18_10); + EXPECT_NE(schema_18_10, schema_18_9); + EXPECT_NE(schema_18_10, schema_17_10); + EXPECT_NE(schema_18_10, schema_17_9); } TEST_F(TestSchema, TestColumnSchemaEquals) { @@ -294,14 +324,14 @@ TEST_F(TestSchema, TestSchemaEquals) { ColumnSchema("col2", UINT32), ColumnSchema("col3", UINT32, false) }, 2); - ASSERT_FALSE(schema1.Equals(schema2)); + ASSERT_NE(schema1, schema2); ASSERT_TRUE(schema1.KeyEquals(schema1)); ASSERT_TRUE(schema1.KeyEquals(schema2, ColumnSchema::COMPARE_TYPE)); ASSERT_FALSE(schema1.KeyEquals(schema2, ColumnSchema::COMPARE_NAME)); ASSERT_TRUE(schema1.KeyTypeEquals(schema2)); ASSERT_FALSE(schema2.KeyTypeEquals(schema3)); - ASSERT_FALSE(schema3.Equals(schema4)); - ASSERT_TRUE(schema4.Equals(schema4)); + ASSERT_NE(schema3, schema4); + ASSERT_EQ(schema4, schema4); ASSERT_TRUE(schema3.KeyEquals(schema4, ColumnSchema::COMPARE_NAME_AND_TYPE)); } @@ -470,7 +500,7 @@ TEST_F(TestSchema, TestGetMappedReadProjection) { ASSERT_EQ(1, mapped.num_key_columns()); ASSERT_EQ(2, mapped.num_columns()); ASSERT_TRUE(mapped.has_column_ids()); - ASSERT_FALSE(mapped.Equals(projection, Schema::COMPARE_ALL)); + ASSERT_FALSE(EqualSchemas(mapped, projection)); // The column id for the 'key' column in the mapped projection should match // the one from the tablet schema. @@ -521,7 +551,7 @@ TEST_F(TestSchema, TestRowOperations) { rb.AddUint32(3); rb.AddInt32(-3); ContiguousRow row_a(&schema); - ASSERT_OK(CopyRowToArena(rb.data(), schema, &arena, &row_a)); + ASSERT_OK(CopyRowToArena(rb.data(), &arena, &row_a)); rb.Reset(); rb.AddString(string("row_b_1")); @@ -529,7 +559,7 @@ TEST_F(TestSchema, TestRowOperations) { rb.AddUint32(3); rb.AddInt32(-3); ContiguousRow row_b(&schema); - ASSERT_OK(CopyRowToArena(rb.data(), schema, &arena, &row_b)); + ASSERT_OK(CopyRowToArena(rb.data(), &arena, &row_b)); ASSERT_GT(schema.Compare(row_b, row_a), 0); ASSERT_LT(schema.Compare(row_a, row_b), 0); diff --git a/src/kudu/common/schema.h b/src/kudu/common/schema.h index c00e66d..725a0e8 100644 --- a/src/kudu/common/schema.h +++ b/src/kudu/common/schema.h @@ -50,12 +50,12 @@ namespace kudu { class Schema; } // namespace kudu -// Check that two schemas are equal, yielding a useful error message in the case that -// they are not. +// Check that two schemas are equal, yielding a useful error message +// if they are not. #define DCHECK_SCHEMA_EQ(s1, s2) \ do { \ - DCHECK((s1).Equals((s2))) << "Schema " << (s1).ToString() \ - << " does not match " << (s2).ToString(); \ + DCHECK((s1) == (s2)) << "Schema " << (s1).ToString() \ + << " does not match " << (s2).ToString(); \ } while (0) #define DCHECK_KEY_PROJECTION_SCHEMA_EQ(s1, s2) \ @@ -787,42 +787,28 @@ class Schema { // so should only be used when necessary for output. std::string ToString(ToStringMode mode = ToStringMode::WITH_COLUMN_IDS) const; - // Compare column ids in Equals() method. - enum SchemaComparisonType { - COMPARE_COLUMNS = 1 << 0, - COMPARE_COLUMN_IDS = 1 << 1, - - COMPARE_ALL = COMPARE_COLUMNS | COMPARE_COLUMN_IDS - }; - - // Return true if the schemas have exactly the same set of columns - // and respective types. - bool Equals(const Schema& other, SchemaComparisonType flags = COMPARE_COLUMNS) const { - if (this == &other) return true; + bool operator==(const Schema& other) const { + if (this == &other) { + return true; + } - if (flags & COMPARE_COLUMNS) { - if (this->num_key_columns_ != other.num_key_columns_) return false; - if (this->num_columns() != other.num_columns()) return false; - for (size_t i = 0; i < other.num_columns(); i++) { - if (!this->cols_[i].Equals(other.cols_[i])) return false; - } + if (this->num_key_columns_ != other.num_key_columns_) { + return false; } - if (flags & COMPARE_COLUMN_IDS) { - if (this->has_column_ids() != other.has_column_ids()) return false; - if (this->has_column_ids()) { - if (this->col_ids_ != other.col_ids_) return false; - if (this->max_col_id() != other.max_col_id()) return false; + const size_t num_columns = this->num_columns(); + if (num_columns != other.num_columns()) { + return false; + } + for (size_t i = 0; i < num_columns; ++i) { + if (!this->cols_[i].Equals(other.cols_[i])) { + return false; } } return true; } - bool operator==(const Schema& other) const { - return this->Equals(other); - } - bool operator!=(const Schema& other) const { return !(*this == other); } @@ -967,6 +953,9 @@ class Schema { friend class SchemaBuilder; + // 'Deep' compare two schemas: this is used by Schema's unit tests. + friend bool EqualSchemas(const Schema&, const Schema&); + std::vector<ColumnSchema> cols_; size_t num_key_columns_; std::vector<ColumnId> col_ids_; diff --git a/src/kudu/integration-tests/alter_table-test.cc b/src/kudu/integration-tests/alter_table-test.cc index 0c2e79f..c356bda 100644 --- a/src/kudu/integration-tests/alter_table-test.cc +++ b/src/kudu/integration-tests/alter_table-test.cc @@ -307,8 +307,8 @@ class AlterTableTest : public KuduTest { } else { ASSERT_EQ(first_node_replica->tablet()->tablet_id(), cur_node_replica->tablet()->tablet_id()); - ASSERT_TRUE(first_node_replica->tablet()->schema()->Equals( - *(cur_node_replica->tablet()->schema()))); + ASSERT_EQ(*(first_node_replica->tablet()->schema()), + *(cur_node_replica->tablet()->schema())); if (verify_row_count == VerifyRowCount::kEnable) { uint64_t cur_count = 0; ASSERT_OK(cur_node_replica->CountLiveRows(&cur_count)); diff --git a/src/kudu/master/master-test.cc b/src/kudu/master/master-test.cc index 289c12f..537686b 100644 --- a/src/kudu/master/master-test.cc +++ b/src/kudu/master/master-test.cc @@ -1398,7 +1398,7 @@ TEST_F(MasterTest, TestGetTableSchemaIsAtomicWithCreateTable) { } else { Schema receivedSchema; CHECK_OK(SchemaFromPB(resp.schema(), &receivedSchema)); - CHECK(kTableSchema.Equals(receivedSchema)) << + CHECK(kTableSchema == receivedSchema) << strings::Substitute("$0 not equal to $1", kTableSchema.ToString(), receivedSchema.ToString()); CHECK_EQ(kTableName, resp.table_name()); diff --git a/src/kudu/master/sys_catalog.cc b/src/kudu/master/sys_catalog.cc index 74662ce..0d417a5 100644 --- a/src/kudu/master/sys_catalog.cc +++ b/src/kudu/master/sys_catalog.cc @@ -252,7 +252,7 @@ Status SysCatalogTable::Load(FsManager *fs_manager) { RETURN_NOT_OK(tablet::TabletMetadata::Load(fs_manager, kSysCatalogTabletId, &metadata)); // Verify that the schema is the current one - if (!metadata->schema().Equals(BuildTableSchema())) { + if (metadata->schema() != BuildTableSchema()) { // TODO: In this case we probably should execute the migration step. return(Status::Corruption("Unexpected schema", metadata->schema().ToString())); } diff --git a/src/kudu/tablet/compaction-test.cc b/src/kudu/tablet/compaction-test.cc index 4ccba4c..654b95c 100644 --- a/src/kudu/tablet/compaction-test.cc +++ b/src/kudu/tablet/compaction-test.cc @@ -184,7 +184,7 @@ class TestCompaction : public KuduRowSetTest { int row_key, int32_t val) { BuildRow(row_key, val); - if (!mrs->schema().Equals(*row_builder_.schema())) { + if (*row_builder_.schema() != mrs->schema()) { // The MemRowSet is not projecting the row, so must be done by the caller RowProjector projector(row_builder_.schema(), &mrs->schema()); uint8_t rowbuf[ContiguousRowHelper::row_size(mrs->schema())]; diff --git a/src/kudu/tablet/tablet.cc b/src/kudu/tablet/tablet.cc index cbeb2ff..28bb0cf 100644 --- a/src/kudu/tablet/tablet.cc +++ b/src/kudu/tablet/tablet.cc @@ -1565,7 +1565,7 @@ Status Tablet::AlterSchema(AlterSchemaOpState* op_state) { std::lock_guard<Semaphore> lock(rowsets_flush_sem_); // If the current version >= new version, there is nothing to do. - bool same_schema = schema()->Equals(*op_state->schema()); + const bool same_schema = (*schema() == *op_state->schema()); if (metadata_->schema_version() >= op_state->schema_version()) { const string msg = Substitute("Skipping requested alter to schema version $0, tablet already " diff --git a/src/kudu/tablet/tablet_metadata.cc b/src/kudu/tablet/tablet_metadata.cc index aabcb7d..0a36f1c 100644 --- a/src/kudu/tablet/tablet_metadata.cc +++ b/src/kudu/tablet/tablet_metadata.cc @@ -167,7 +167,7 @@ Status TabletMetadata::LoadOrCreate(FsManager* fs_manager, scoped_refptr<TabletMetadata>* metadata) { Status s = Load(fs_manager, tablet_id, metadata); if (s.ok()) { - if (!(*metadata)->schema().Equals(schema)) { + if ((*metadata)->schema() != schema) { return Status::Corruption(Substitute("Schema on disk ($0) does not " "match expected schema ($1)", (*metadata)->schema().ToString(), schema.ToString())); diff --git a/src/kudu/tserver/tablet_service.cc b/src/kudu/tserver/tablet_service.cc index d37bf08..16c73ba 100644 --- a/src/kudu/tserver/tablet_service.cc +++ b/src/kudu/tserver/tablet_service.cc @@ -1170,8 +1170,8 @@ void TabletServiceAdminImpl::AlterSchema(const AlterSchemaRequestPB* req, return; } - Schema tablet_schema = replica->tablet_metadata()->schema(); - if (req_schema.Equals(tablet_schema)) { + const auto& tablet_schema = replica->tablet_metadata()->schema(); + if (req_schema == tablet_schema) { context->RespondSuccess(); return; }
