Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17890 )
Change subject: KUDU-2671 introduce PartitionKey ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/17890/3/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/17890/3/src/kudu/common/partition.h@102 PS3, Line 102: other < *this; > nit: switch order to *this > other; That would be an infinite recursion. The idea is to express operator>() via already defined operator<(), hence the order. http://gerrit.cloudera.org:8080/#/c/17890/3/src/kudu/common/partition.h@106 PS3, Line 106: return other < *this || other == *this; > nit: same as above. Yup, here it's possible to use the newly defined operator>() since the latter has just been defined above. http://gerrit.cloudera.org:8080/#/c/17890/3/src/kudu/common/partition_pruner-test.cc File src/kudu/common/partition_pruner-test.cc: http://gerrit.cloudera.org:8080/#/c/17890/3/src/kudu/common/partition_pruner-test.cc@1260 PS3, Line 1260: 12 - 5 > 12 - 5 = 7 so isn't this the result we're expecting? Ah, that seems to be a typo. Thanks! http://gerrit.cloudera.org:8080/#/c/17890/3/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/17890/3/src/kudu/master/master.proto@621 PS3, Line 621: message TablePartitionKeyPB { > Would it make sense to split up this protobuf into one message just encapsu I think that's a good idea -- that way the new type will mirror PartitionKey in the code. 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: 3 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Sat, 02 Oct 2021 04:15:08 +0000 Gerrit-HasComments: Yes
