Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17890 )
Change subject: KUDU-2671 introduce PartitionKey ...................................................................... Patch Set 4: (14 comments) http://gerrit.cloudera.org:8080/#/c/17890/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17890/4//COMMIT_MSG@29 PS4, Line 29: As a result, a few new tests scenarios related to using custom hash : schemas per range partition is not working as expected yet because they : require the comparison operator for PartitionKey to be updated: > I think I missed something -- was there a functional change that led to thi Good point -- actually, the comparison operator has changed a bit and that's why the difference: the corner case is visible in case when there are ranges with no hash schema with encoded range key greater than any encoded key with at least one hash bucket schema. Instead of explaining this difference here in the description, I decided to make the comparison operator for PartitionKey really equivalent to the legacy comparison, and now those tests pass. http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/common/partition.h@50 PS4, Line 50: class PartitionKey { > nit: maybe this will change based on the ordering, but maybe worth eventual Done http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/common/partition.h@52 PS4, Line 52: PartitionKey() = default; > nit: maybe document that this is considered an "-Inf" key, and is valid to Done http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/common/partition.h@92 PS4, Line 92: *this > other || *this == other; > nit: maybe just !(this < other)? Same with <=? Done http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/common/partition.h@112 PS4, Line 112: redable > nit: readable Done http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/common/partition.cc@108 PS4, Line 108: ranges > nit: I suppose this makes sense too, but to avoid overloading "range" too m Good point. I removed this DCHECK along with the comment since I found Partition is used in some hacky ways in tests. http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/common/partition.cc@627 PS4, Line 627: )); > nit: maybe print the range keys? Done http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/common/partition.cc@664 PS4, Line 664: // TODO(aserbin): is this really necessary -- zeros in hash key are the : // minimum possible values, and the range part is already : // empty? > This is an interesting observation -- there doesn't seem to be an functiona Yep, it seems this was just one of various preparations when building sets/maps of ordered partition keys. I think to remove this in a separate changelist after making sure I'm not missing some important point here. http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/master/catalog_manager-test.cc File src/kudu/master/catalog_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/master/catalog_manager-test.cc@101 PS4, Line 101: TEST(TableInfoTest, GetTableLocationsLegacyCustomHashSchemas) { > Would it make sense to add a test with multiple ranges and have the start a Yeah -- those are to be added in a follow-up patch to verify the functionality in the end-to-end manner. This small unit-test is here to verify one particular point of GetTabletsInRange() implementation: it returns non-OK status when not supplying the range information properly for a table with custom hash partitioning schemas per range. http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/master/catalog_manager.h@304 PS4, Line 304: only a > nit: "This is only applicable..." Done http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/master/catalog_manager.cc@5697 PS4, Line 5697: PREDICT_FALSE(req->has_key_start() && req->has_key_end() && : req->key_start().has_range_key() && : req->key_end().has_range_key() && : req->key_start().range_key() > req->key_end().range_key())) { : return Status::InvalidArgument("start partition range key must not be " : "greater than the end partition range key"); : } > Is it worth also checking if key_end doesn't have a range key but key_start Yep, there are some options here: we might consider absent range key part as an empty range key, i.e. inf. I'd rather postpone checking for invariants for a follow-up changelist -- I guess we might add those uniform checks in several places to validating the input data. http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/master/catalog_manager.cc@6394 PS4, Line 6394: , > nit: space after comma Well, this is shell-like notation (particularly, Bourne shell). Adding a space breaks it in shell (i.e. in shell you shouldn't add a space there), so I'd either keep it as. Let me know if you feel strongly about this -- I can update the comment and remove this shell-ism. http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/master/master.proto@649 PS4, Line 649: fields obsolete the : // 'partition_key_start' and 'partition_key_end' fields above > nit: "these fields make the ... fields above obsolete" Done http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/master/master.proto@654 PS4, Line 654: encoraged > encouraged. Done -- To view, visit http://gerrit.cloudera.org:8080/17890 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00255ec404beeb999117f5265de0d5d8deaf0d68 Gerrit-Change-Number: 17890 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 06 Oct 2021 05:15:23 +0000 Gerrit-HasComments: Yes
