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 8: (10 comments) http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.h@167 PS8, Line 167: const google::protobuf::RepeatedPtrField : <PartitionSchemaPB_ColumnIdentifierPB>& identifiers, > Nit: Might be better to move the entire argument declaration on separate ne Ack http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@124 PS8, Line 124: const RepeatedPtrField : <PartitionSchemaPB_ColumnIdentifierPB>& identifiers, > Nit: Same as above. Ack http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@128 PS8, Line 128: column_ids > +1. Ack http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@136 PS8, Line 136: column_id > Nit: std::move(column_id) Ack http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@144 PS8, Line 144: push_back > Nit: emplace_back instead. Ack http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@375 PS8, Line 375: void > If no error status is expected then better to return hash_partitions instea Ack http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@378 PS8, Line 378: vector<Partition> hash_partitions_so_far = vector<Partition>(1) > Assignment/copy constructor is unnecessary. Ack http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@382 PS8, Line 382: // For each of the partitions created so far, replicate it > Looks like the size of new_partitions can be predetermined. Ack http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@404 PS8, Line 404: KeyEncoder<string> > Nit: auto Ack http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@459 PS8, Line 459: current_bound_hash_partitions.begin(), : current_bound_hash_partitions.end() > Nit: Could use std::make_move_iterator() to save the copies. 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: 8 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, 26 Nov 2020 01:52:37 +0000 Gerrit-HasComments: Yes
