Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16859 )
Change subject: [master] Range specific hashing at table creation time. ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/common/partition-test.cc@1157 PS4, Line 1157: EXPECT_EQ(0, partitions[0].hash_buckets()[0]); nit: would it be possible to write this in a similar style to TableLocationsTest.TestRangeSpecificHashing? IMO it's much easier to understand and verify correctness. If you want to continue checking the hash buckets, you could even put the hash key, range keys, and partition key into a struct; form the expected vector of structs; sort that by the partition key start; and then compare. http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/16859/3/src/kudu/common/partition.cc@417 PS3, Line 417: : if (!split_rows.empty()) { : for (const auto& hash_schemas : range_hash_schemas) { : if (!hash_schemas.empty()) { : return Status::InvalidArgument("Both 'split_rows' and 'range_hash_schemas' cannot be " : "populated at the same time."); : } : } : } : : if (!range_hash_schemas.empty() && range_bounds.size() != range_hash_schemas.size()) { : return Status::InvalidArgument("The number of range bounds does not match the number of user " : "supplied hash schemas."); : } > nope, I can add one. Should I add another one at a higher level (in table_l If there are unexercised code paths, more coverage couldn't hurt. That said, I expect even more test coverage of these paths when your client patch lands anyway. http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/integration-tests/table_locations-itest.cc File src/kudu/integration-tests/table_locations-itest.cc: http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/integration-tests/table_locations-itest.cc@482 PS4, Line 482: { nit: there doesn't seem to be a point to this extra scope, since we're only sending one 'req'. Other tests use this to avoid naming collisions, which isn't an issue here. http://gerrit.cloudera.org:8080/#/c/16859/4/src/kudu/integration-tests/table_locations-itest.cc@517 PS4, Line 517: [](const string& s1, const string& s2) { : return s1.compare(s2) < 0; : }); nit: Isn't this the default comparator for strings? Can we get by without it? -- To view, visit http://gerrit.cloudera.org:8080/16859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f0dcbc3324f8f2d6e99b4d169fdf5c7f7dff95d Gerrit-Change-Number: 16859 Gerrit-PatchSet: 4 Gerrit-Owner: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Comment-Date: Wed, 16 Dec 2020 02:24:03 +0000 Gerrit-HasComments: Yes
