Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16596 )
Change subject: [partitioning] KUDU-2671: Support for range specific HashSchemas. ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.h@149 PS1, Line 149: integer > right now it's useless, but the idea was to replace the integer with the up I guess it would be enough to have just the lower bound for a range given that we don't allow ranges to intersect, right? Also, why is it 'int', not 'std::string' as the type of the Partition::partition_key_start_ member field would imply? http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@366 PS1, Line 366: partitions > nit: not your fault, but IMO it makes it more difficult to read to use 'par +1 http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@398 PS1, Line 398: RETURN_NOT_OK(EncodeRangeBounds(range_bounds, schema, &bounds)); : RETURN_NOT_OK(EncodeRangeSplits(split_rows, schema, &splits)); : RETURN_NOT_OK(SplitRangeBounds(schema, std::move(splits), &bounds)); > If we don't have any 'split_rows', then the number of bounds would be known style nit: it seems there three lines has shifted right; what might be the reason? http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@410 PS1, Line 410: vector<Partition> current_bound_hash_partitions = vector<Partition>(1); nit: a shorter form might be vector<Partition> current_bound_hash_partitions(1); http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@669 PS1, Line 669: string PartitionSchema::HashSchemaPerPartitionDebugString(const Partition& partition, : const vector<HashBucketSchema>& range_hash_schema, : const Schema& schema) const { style nit: the parameters are not aligned properly http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@675 PS1, Line 675: nit: reserving the space for 'components' might be a good idea; I guess here it'd be components.reserve(hash_bucket_schemas_.size() + 1); http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@686 PS1, Line 686: use default HashSchema when no range-specific hash partitions are provided style nit: in comments like this, in Kudu code we tend to use full sentences, i.e. it should start with a capital letter and end with a period (dot). http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@695 PS1, Line 695: hash_buckets_[i] How is it guaranteed that partition.hash_buckets_ has enough elements to have index 'i' to be in the bounds of the container? -- 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: 1 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-Comment-Date: Wed, 14 Oct 2020 23:09:37 +0000 Gerrit-HasComments: Yes
