This is an automated email from the ASF dual-hosted git repository. bankim pushed a commit to branch branch-1.15.x in repository https://gitbox.apache.org/repos/asf/kudu.git
commit 3fefe5fd27e163111d89427ef488e5c422e4b6d2 Author: Alexey Serbin <[email protected]> AuthorDate: Mon Jun 7 18:21:26 2021 -0700 [client] replace Equals() with operator==() This patch replaces the 'bool Equals(const X&)' method for Partition and PartitionSchema classes with 'bool operator==(const X&)'. For KuduSchema and KuduColumnSchema, both operator==() and operator!=() were introduced, and the Equals() method was marked as deprecated since KuduSchema's and KuduColumnsSchema's API are public at this point. I decided to perform minor refactoring while changing other code around: the presence of Equals() method looks a bit lame in a C++ API. This patch doesn't contain any functional modifications. Change-Id: I4a95534835c93b1fb7a3ca1f311c530572c50ad2 Reviewed-on: http://gerrit.cloudera.org:8080/17558 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong <[email protected]> (cherry picked from commit 79bd57546996c6e3c130960ba099fb637d58d51f) Reviewed-on: http://gerrit.cloudera.org:8080/17563 Reviewed-by: Bankim Bhavsar <[email protected]> --- src/kudu/client/client-test.cc | 4 +-- src/kudu/client/client-unittest.cc | 11 +++--- src/kudu/client/schema.cc | 25 ++++++++++--- src/kudu/client/schema.h | 42 ++++++++++++++++++++-- src/kudu/common/partition-test.cc | 4 +-- src/kudu/common/partition.cc | 50 ++++++++++++++++++-------- src/kudu/common/partition.h | 8 ++--- src/kudu/integration-tests/alter_table-test.cc | 2 +- src/kudu/integration-tests/test_workload.cc | 2 +- src/kudu/tablet/tablet_metadata.cc | 4 +-- src/kudu/tools/table_scanner.cc | 9 ++--- 11 files changed, 117 insertions(+), 44 deletions(-) diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc index d0ee1c0..2dd82a0 100644 --- a/src/kudu/client/client-test.cc +++ b/src/kudu/client/client-test.cc @@ -4895,11 +4895,11 @@ TEST_F(ClientTest, TestGetTableSchema) { // Verify the schema for the current table ASSERT_OK(client_->GetTableSchema(kTableName, &schema)); - ASSERT_TRUE(schema_.Equals(schema)); + ASSERT_EQ(schema_, schema); // Verify that a get schema request for a missing table throws not found Status s = client_->GetTableSchema("MissingTableName", &schema); - ASSERT_TRUE(s.IsNotFound()); + ASSERT_TRUE(s.IsNotFound()) << s.ToString(); ASSERT_STR_CONTAINS(s.ToString(), "the table does not exist"); } diff --git a/src/kudu/client/client-unittest.cc b/src/kudu/client/client-unittest.cc index 5c80386..2c0fb01 100644 --- a/src/kudu/client/client-unittest.cc +++ b/src/kudu/client/client-unittest.cc @@ -318,21 +318,22 @@ TEST(ClientUnitTest, TestKuduSchemaToStringWithColumnIds) { TEST(KuduColumnSchemaTest, TestEquals) { KuduColumnSchema a32("a", KuduColumnSchema::INT32); - ASSERT_TRUE(a32.Equals(a32)); + ASSERT_TRUE(a32 == a32); KuduColumnSchema a32_2(a32); - ASSERT_TRUE(a32.Equals(a32_2)); + ASSERT_EQ(a32, a32_2); KuduColumnSchema b32("b", KuduColumnSchema::INT32); - ASSERT_FALSE(a32.Equals(b32)); + ASSERT_FALSE(a32 == b32); + ASSERT_TRUE(a32 != b32); KuduColumnSchema a16("a", KuduColumnSchema::INT16); - ASSERT_FALSE(a32.Equals(a16)); + ASSERT_NE(a32, a16); const int kDefaultOf7 = 7; KuduColumnSchema a32_dflt("a", KuduColumnSchema::INT32, /*is_nullable=*/false, /*default_value=*/&kDefaultOf7); - ASSERT_FALSE(a32.Equals(a32_dflt)); + ASSERT_NE(a32, a32_dflt); } } // namespace client diff --git a/src/kudu/client/schema.cc b/src/kudu/client/schema.cc index 66085bb..4a9f1c3 100644 --- a/src/kudu/client/schema.cc +++ b/src/kudu/client/schema.cc @@ -778,9 +778,16 @@ void KuduColumnSchema::CopyFrom(const KuduColumnSchema& other) { } bool KuduColumnSchema::Equals(const KuduColumnSchema& other) const { - return this == &other || - col_ == other.col_ || - (col_ != nullptr && col_->Equals(*other.col_, ColumnSchema::COMPARE_ALL)); + return *this == other; +} + +bool KuduColumnSchema::operator==(const KuduColumnSchema& rhs) const { + return this == &rhs || col_ == rhs.col_ || + (col_ != nullptr && col_->Equals(*rhs.col_, ColumnSchema::COMPARE_ALL)); +} + +bool KuduColumnSchema::operator!=(const KuduColumnSchema& rhs) const { + return !(*this == rhs); } const string& KuduColumnSchema::name() const { @@ -856,9 +863,17 @@ Status KuduSchema::Reset(const vector<KuduColumnSchema>& columns, int key_column return Status::OK(); } +bool KuduSchema::operator==(const KuduSchema& rhs) const { + return this == &rhs || + (schema_ && rhs.schema_ && schema_->Equals(*rhs.schema_)); +} + +bool KuduSchema::operator!=(const KuduSchema& rhs) const { + return !(this == &rhs); +} + bool KuduSchema::Equals(const KuduSchema& other) const { - return this == &other || - (schema_ && other.schema_ && schema_->Equals(*other.schema_)); + return *this == other; } KuduColumnSchema KuduSchema::Column(size_t idx) const { diff --git a/src/kudu/client/schema.h b/src/kudu/client/schema.h index 7e3f2a9..086e7d9 100644 --- a/src/kudu/client/schema.h +++ b/src/kudu/client/schema.h @@ -265,10 +265,29 @@ class KUDU_EXPORT KuduColumnSchema { /// Check whether the object is identical to the other one. /// + /// @deprecated use operator==(const KuduColumnSchema&) instead + /// /// @param [in] other /// The reference object to compare with. /// @return @c true iff the object is identical to the specified one. - bool Equals(const KuduColumnSchema& other) const; + bool Equals(const KuduColumnSchema& other) const + ATTRIBUTE_DEPRECATED("use operator==(const KuduColumnSchema&) instead"); + + /// Check whether the schema is identical to the other one. + /// + /// @param [in] rhs + /// KuduColumnSchema object to compare this one with. + /// @return @c true iff this KuduColumnSchema object is identical + /// to the specified one. + bool operator==(const KuduColumnSchema& rhs) const; + + /// Check whether the schema is not identical to the other one. + /// + /// @param [in] rhs + /// KuduColumnSchema object to compare this one with. + /// @return @c true iff this KuduColumnSchema object is not identical + /// to the specified one. + bool operator!=(const KuduColumnSchema& rhs) const; /// @name Getters to expose column schema information. /// @@ -633,11 +652,30 @@ class KUDU_EXPORT KuduSchema { /// Check whether the schema is identical to the other one. /// + /// @deprecated use operator==(const KuduSchema&) instead + /// /// @param [in] other /// The other KuduSchema object to compare with. /// @return @c true iff this KuduSchema object is identical /// to the specified one. - bool Equals(const KuduSchema& other) const; + bool Equals(const KuduSchema& other) const + ATTRIBUTE_DEPRECATED("use operator==(const KuduSchema&) instead"); + + /// Check whether the schema is identical to the other one. + /// + /// @param [in] rhs + /// KuduSchema object to compare this one with. + /// @return @c true iff this KuduSchema object is identical + /// to the specified one. + bool operator==(const KuduSchema& rhs) const; + + /// Check whether the schema is not identical to the other one. + /// + /// @param [in] rhs + /// KuduSchema object to compare this one with. + /// @return @c true iff this KuduSchema object is not identical + /// to the specified one. + bool operator!=(const KuduSchema& rhs) const; /// @param [in] idx /// Column index. diff --git a/src/kudu/common/partition-test.cc b/src/kudu/common/partition-test.cc index e71cce0..5b63260 100644 --- a/src/kudu/common/partition-test.cc +++ b/src/kudu/common/partition-test.cc @@ -982,7 +982,6 @@ void CheckPartitions(const vector<Partition>& partitions) { void CheckSerializationFunctions(const PartitionSchemaPB& pb, const PartitionSchema& partition_schema, const Schema& schema) { - PartitionSchemaPB pb1; ASSERT_OK(partition_schema.ToPB(schema, &pb1)); @@ -991,8 +990,7 @@ void CheckSerializationFunctions(const PartitionSchemaPB& pb, PartitionSchema partition_schema1; ASSERT_OK(PartitionSchema::FromPB(pb1, schema, &partition_schema1)); - - ASSERT_TRUE(partition_schema.Equals(partition_schema1)); + ASSERT_EQ(partition_schema, partition_schema1); } } // namespace diff --git a/src/kudu/common/partition.cc b/src/kudu/common/partition.cc index e7787d0..6aa5cb5 100644 --- a/src/kudu/common/partition.cc +++ b/src/kudu/common/partition.cc @@ -84,11 +84,19 @@ Slice Partition::range_key(const string& partition_key) const { } } -bool Partition::Equals(const Partition& other) const { - if (this == &other) return true; - if (partition_key_start() != other.partition_key_start()) return false; - if (partition_key_end() != other.partition_key_end()) return false; - if (hash_buckets_ != other.hash_buckets_) return false; +bool Partition::operator==(const Partition& rhs) const { + if (this == &rhs) { + return true; + } + if (partition_key_start() != rhs.partition_key_start()) { + return false; + } + if (partition_key_end() != rhs.partition_key_end()) { + return false; + } + if (hash_buckets_ != rhs.hash_buckets_) { + return false; + } return true; } @@ -1098,20 +1106,32 @@ string PartitionSchema::PartitionTableEntry(const Schema& schema, return entry; } -bool PartitionSchema::Equals(const PartitionSchema& other) const { - if (this == &other) return true; +bool PartitionSchema::operator==(const PartitionSchema& rhs) const { + if (this == &rhs) { + return true; + } // Compare range component. - if (range_schema_.column_ids != other.range_schema_.column_ids) return false; + if (range_schema_.column_ids != rhs.range_schema_.column_ids) { + return false; + } // Compare hash bucket components. - if (hash_bucket_schemas_.size() != other.hash_bucket_schemas_.size()) return false; - for (int i = 0; i < hash_bucket_schemas_.size(); i++) { - if (hash_bucket_schemas_[i].seed != other.hash_bucket_schemas_[i].seed) return false; - if (hash_bucket_schemas_[i].num_buckets - != other.hash_bucket_schemas_[i].num_buckets) return false; - if (hash_bucket_schemas_[i].column_ids - != other.hash_bucket_schemas_[i].column_ids) return false; + const auto& hb_schemas = hash_bucket_schemas_; + const auto& rhs_hb_schemas = rhs.hash_bucket_schemas_; + if (hb_schemas.size() != rhs_hb_schemas.size()) { + return false; + } + for (size_t i = 0; i < hb_schemas.size(); ++i) { + if (hb_schemas[i].seed != rhs_hb_schemas[i].seed) { + return false; + } + if (hb_schemas[i].num_buckets != rhs_hb_schemas[i].num_buckets) { + return false; + } + if (hb_schemas[i].column_ids != rhs_hb_schemas[i].column_ids) { + return false; + } } return true; diff --git a/src/kudu/common/partition.h b/src/kudu/common/partition.h index 2226e45..c37c7c5 100644 --- a/src/kudu/common/partition.h +++ b/src/kudu/common/partition.h @@ -73,8 +73,8 @@ class Partition { return partition_key_end_; } - // Returns true if the other partition is equivalent to this one. - bool Equals(const Partition& other) const; + // Returns true iff the given partition 'rhs' is equivalent to this one. + bool operator==(const Partition& rhs) const; // Serializes a partition into a protobuf message. void ToPB(PartitionPB* pb) const; @@ -281,8 +281,8 @@ class PartitionSchema { std::string PartitionTableHeader(const Schema& schema) const; std::string PartitionTableEntry(const Schema& schema, const Partition& partition) const; - // Returns true if the other partition schema is equivalent to this one. - bool Equals(const PartitionSchema& other) const; + // Returns 'true' iff the partition schema 'rhs' is equivalent to this one. + bool operator==(const PartitionSchema& rhs) const; // Transforms an exclusive lower bound range partition key into an inclusive // lower bound range partition key. diff --git a/src/kudu/integration-tests/alter_table-test.cc b/src/kudu/integration-tests/alter_table-test.cc index 42e4818..09bcb05 100644 --- a/src/kudu/integration-tests/alter_table-test.cc +++ b/src/kudu/integration-tests/alter_table-test.cc @@ -556,7 +556,7 @@ TEST_F(AlterTableTest, TestAlterOnTSRestart) { KuduSchema schema; bool alter_in_progress = false; ASSERT_OK(client_->GetTableSchema(kTableName, &schema)); - ASSERT_TRUE(schema_.Equals(schema)); + ASSERT_EQ(schema_, schema); ASSERT_OK(client_->IsAlterTableInProgress(kTableName, &alter_in_progress)); ASSERT_TRUE(alter_in_progress); diff --git a/src/kudu/integration-tests/test_workload.cc b/src/kudu/integration-tests/test_workload.cc index 51e089a..d59b04e 100644 --- a/src/kudu/integration-tests/test_workload.cc +++ b/src/kudu/integration-tests/test_workload.cc @@ -392,7 +392,7 @@ void TestWorkload::Setup() { } else { KuduSchema existing_schema; CHECK_OK(client_->GetTableSchema(table_name_, &existing_schema)); - CHECK(schema_.Equals(existing_schema)) + CHECK(schema_ == existing_schema) << "Existing table's schema doesn't match ours"; LOG(INFO) << "TestWorkload: Skipping table creation because table " << table_name_ << " already exists"; diff --git a/src/kudu/tablet/tablet_metadata.cc b/src/kudu/tablet/tablet_metadata.cc index 6e635a5..809a5c9 100644 --- a/src/kudu/tablet/tablet_metadata.cc +++ b/src/kudu/tablet/tablet_metadata.cc @@ -415,11 +415,11 @@ Status TabletMetadata::LoadFromSuperBlock(const TabletSuperBlockPB& superblock) PartitionSchema partition_schema; RETURN_NOT_OK(PartitionSchema::FromPB(superblock.partition_schema(), *schema_, &partition_schema)); - CHECK(partition_schema_.Equals(partition_schema)); + CHECK(partition_schema_ == partition_schema); Partition partition; Partition::FromPB(superblock.partition(), &partition); - CHECK(partition_.Equals(partition)); + CHECK(partition_ == partition); } tablet_data_state_ = superblock.tablet_data_state(); diff --git a/src/kudu/tools/table_scanner.cc b/src/kudu/tools/table_scanner.cc index 3fa89cd..d88b3c1 100644 --- a/src/kudu/tools/table_scanner.cc +++ b/src/kudu/tools/table_scanner.cc @@ -337,7 +337,7 @@ Status CreateDstTableIfNeeded(const client::sp::shared_ptr<KuduTable>& src_table const string& dst_table_name) { client::sp::shared_ptr<KuduTable> dst_table; Status s = dst_client->OpenTable(dst_table_name, &dst_table); - if (!s.IsNotFound() && !s.ok()) { + if (!s.IsNotFound()) { return s; } @@ -349,9 +349,10 @@ Status CreateDstTableIfNeeded(const client::sp::shared_ptr<KuduTable>& src_table } const KuduSchema& dst_table_schema = dst_table->schema(); - if (!src_table_schema.Equals(dst_table_schema)) { - return Status::NotSupported( - "Not support different schema of source table and destination table."); + if (src_table_schema != dst_table_schema) { + return Status::NotSupported(Substitute( + "destination table's schema differs from the source one ($0 vs $1)", + dst_table_schema.ToString(), src_table_schema.ToString())); } return Status::OK();
