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 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/16596/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16596/1//COMMIT_MSG@12 PS1, Line 12: Open to discussion on whether or : not we should support this feature with split_rows. > Done will add a section for this in design doc 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 > I guess it would be enough to have just the lower bound for a range given t hm yeh just the lower bound should suffice. Right now, the 'int' is just a placeholder for the bound that should replace it. In hindsight, using an 'int' wasn't very clear and perhaps I should have used 'std::string' instead. 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 > +1 Done 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)); > style nit: it seems there three lines has shifted right; what might be the yeh I noticed that as soon as I uploaded the patch, fixed it for next one 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 Done http://gerrit.cloudera.org:8080/#/c/16596/1/src/kudu/common/partition.cc@413 PS1, Line 413: second > answered in definition of 'range_hash_schema' in header file Done 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 Done 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 her Done 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 sentence Done 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 ha The for loop runs through the hash schemas that were applied to the current partition. The current partition will have as many elements in 'partition.hash_buckets_' as the hash schemas because the amount of necessary hash buckets has already been generated from the hash schemas per each range. For example, if two different hash schemas apply to a range, a partition with two different hash buckets will be created. -- 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: Thu, 22 Oct 2020 23:15:47 +0000 Gerrit-HasComments: Yes
