Bankim Bhavsar 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 next line so that it fits on one line. 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. http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@128 PS8, Line 128: column_ids > Ack +1. http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@136 PS8, Line 136: column_id Nit: std::move(column_id) http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@144 PS8, Line 144: push_back Nit: emplace_back instead. 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 instead of using it as out parameter. 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. vector<Partition> hash_partitions_so_far(1) 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. hash_partitions_so_far.size() * bucket.schema.num_buckets so can use new_partitions.reserve() with that size. http://gerrit.cloudera.org:8080/#/c/16596/8/src/kudu/common/partition.cc@404 PS8, Line 404: KeyEncoder<string> Nit: auto 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. -- 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: Wed, 25 Nov 2020 21:15:02 +0000 Gerrit-HasComments: Yes
