Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/16596 )
Change subject: [partitioning] KUDU-2671: Support for range specific HashSchemas. ...................................................................... Patch Set 11: (4 comments) http://gerrit.cloudera.org:8080/#/c/16596/11/src/kudu/common/partition-test.cc File src/kudu/common/partition-test.cc: http://gerrit.cloudera.org:8080/#/c/16596/11/src/kudu/common/partition-test.cc@1073 PS11, Line 1073: range_hash_schemas.emplace_back(PartitionSchema::HashBucketSchemas()); > Is this important? If not, maybe remove it? Seems like it shouldn't be sinc I added it b/c there's a check for the size of 'range_hash_schemas' to match the size of the eventual bounds, but that check happens after the check below that will return an Invalid argument message. In short, we will never reach this size check if the below check is triggered properly so yes I can remove it. http://gerrit.cloudera.org:8080/#/c/16596/11/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/16596/11/src/kudu/common/partition.h@413 PS11, Line 413: encoded_bounds > nit: can you update this? Ack http://gerrit.cloudera.org:8080/#/c/16596/11/src/kudu/common/partition.h@416 PS11, Line 416: range_hash_schemas > nit: can you also document how this is used, what to expect if it's empty, Ack http://gerrit.cloudera.org:8080/#/c/16596/11/src/kudu/common/partition.h@420 PS11, Line 420: // Splits the encoded range bounds by the split points. The splits and bounds : // must be sorted. If `bounds` is empty, then a single unbounded range is : // assumed. If any of the splits falls outside of the bounds then an : // InvalidArgument status is returned. > nit: can you update this? Ack -- To view, visit http://gerrit.cloudera.org:8080/16596 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8725f4bd072a81b05b36dfc7df0c074c172b4ce8 Gerrit-Change-Number: 16596 Gerrit-PatchSet: 11 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 30 Nov 2020 23:49:27 +0000 Gerrit-HasComments: Yes
