Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17753 )
Change subject: [common] Update PartitionSchema::operator==() ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/17753/2/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: http://gerrit.cloudera.org:8080/#/c/17753/2/src/kudu/common/partition-test.cc@1573 PS2, Line 1573: ASSERT_TRUE nit here and below for ASSERT_TRUE(... == ...): ASSERT_TRUE() might be replaced with ASSERT_EQ(), if it makes sense http://gerrit.cloudera.org:8080/#/c/17753/2/src/kudu/common/partition-test.cc@1573 PS2, Line 1573: ASSERT_TRUE(partition_schema == partition_schema); : ASSERT_TRUE(partition_schema_1 == partition_schema_1); Does it make sense to add ASSERT_EQ(partition_schema, partition_schema_1); as well? http://gerrit.cloudera.org:8080/#/c/17753/2/src/kudu/common/partition-test.cc@1579 PS2, Line 1579: ASSERT_FALSE nit here and below for ASSERT_FALSE(... == ...): I guess it would be possible to use ASSERT_NE() here if defining operator!=() http://gerrit.cloudera.org:8080/#/c/17753/2/src/kudu/common/partition-test.cc@1585 PS2, Line 1585: { "a"} nit here and below: add a trailing space or drop the space in the beginning http://gerrit.cloudera.org:8080/#/c/17753/2/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/17753/2/src/kudu/common/partition.h@155 PS2, Line 155: rhs_hb_schema nit: would 'rhs' be more concise here? http://gerrit.cloudera.org:8080/#/c/17753/2/src/kudu/common/partition.h@155 PS2, Line 155: bool operator== It seems the code uses mostly operator!=(const HashBucketSchema& rhs), so maybe introduce that operator instead or just add operator!=() expressed via the logical negation of operator==()? http://gerrit.cloudera.org:8080/#/c/17753/2/src/kudu/common/partition.h@159 PS2, Line 159: this-> nit here and below: maybe, drop 'this'? Or that's something you want to be present for some reason? -- To view, visit http://gerrit.cloudera.org:8080/17753 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09c626ecdcbe7c0465e6483d6a23369fb7194bfb Gerrit-Change-Number: 17753 Gerrit-PatchSet: 2 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Thu, 05 Aug 2021 03:14:57 +0000 Gerrit-HasComments: Yes
