Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16596 )
Change subject: WIP [partitioning] KUDU-2671: Support for range specific HashSchemas. ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/16596/4/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/16596/4/src/kudu/common/partition.cc@276 PS4, Line 276: range_partitions_with_hash_schemas nit: add four spaces here since it's a continuation of the previous line http://gerrit.cloudera.org:8080/#/c/16596/4/src/kudu/common/partition.cc@295 PS4, Line 295: range_hash_schema[j++] What if this is empty? http://gerrit.cloudera.org:8080/#/c/16596/4/src/kudu/common/partition.cc@302 PS4, Line 302: &t1 nit: put the & with the rest of the type; same below http://gerrit.cloudera.org:8080/#/c/16596/4/src/kudu/common/partition.cc@418 PS4, Line 418: RETURN_NOT_OK(EncodeRangeBounds(range_bounds, range_hash_schema, schema, &bounds, : &bounds_with_hash_schemas)); : RETURN_NOT_OK(EncodeRangeSplits(split_rows, schema, &splits)); : RETURN_NOT_OK(SplitRangeBounds(schema, std::move(splits), &bounds)); We chatted on Slack about maybe using a struct{string,string,optional<HashBucketSchemas>} (or just use empty to signify the default hash bucket schema) instead of a tuple, and passing a single 'bounds_with_hash_schemas' around instead of duplicating the 'bounds'. Upon looking around at the usages here, I think it's worth moving forward with both of these changes: EncodeRangeBounds(), EncodeRangeSplits(), and SplitRangeBounds() are all used entirely in this file, so the changes to their API won't have a very wide footprint. I don't recall what reasons there were for the duplication, but it isn't clear why that wouldn't be doable. -- 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: 4 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: Thu, 19 Nov 2020 05:26:28 +0000 Gerrit-HasComments: Yes
