Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17890 )
Change subject: KUDU-2671 introduce PartitionKey ...................................................................... Patch Set 4: (11 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 this behavior other than the debug string implementations? I'm confused as to why these no longer pass, given the PartitionKey comparison operators abide by the existing rules. 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 eventually adding a description, like "Key that identifies the partition that a row belongs to. Rows that belong to a given partition all fall within the same partition key range." 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 be used 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 <=? http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/common/partition.h@112 PS4, Line 112: redable nit: readable 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 much, maybe stick with "buckets"? http://gerrit.cloudera.org:8080/#/c/17890/4/src/kudu/common/partition.cc@627 PS4, Line 627: )); nit: maybe print the range keys? 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 functional difference between (1, _, _) and (1, 0, "") as far as partitioning is concerned, since it seems incorrect/buggy to operate with partition keys that fall in between (1, _, _) and (1, 0, ""). 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..." 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 does? In general, are there other constraints that are worth documenting for key_start and key_end? Though perhaps it's worth waiting until the usage logic stabilizes. 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" -- 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: Tue, 05 Oct 2021 01:36:44 +0000 Gerrit-HasComments: Yes
