Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] wip KUDU-2671: Modifies pruning logic to include per range hash schemas. ...................................................................... Patch Set 1: (10 comments) Did a quick first pass http://gerrit.cloudera.org:8080/#/c/17643/1//COMMIT_MSG Commit Message: PS1: nit: it would be nice to reformat this commit description to follow the guideline outlined at https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines (refereed from https://kudu.apache.org/docs/contributing.html#_submitting_patches) http://gerrit.cloudera.org:8080/#/c/17643/1//COMMIT_MSG@11 PS1, Line 11: same set of hash bucket schemas is used nit: same table-wide set of hash bucket schemas is used for every range http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition.h@285 PS1, Line 285: hb_schemas nit: if the other parameter named 'rhs_hb_schemas', maybe name this one as 'lhs_hb_schemas'? http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition.cc@1108 PS1, Line 1108: : bool PartitionSchema::EqualHashBucketSchemas(const HashBucketSchemas& hb_schemas, : const HashBucketSchemas& rhs_hb_schemas) { > Seems like this could be separated out from the pruning-related bits of thi +1 http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.h File src/kudu/common/partition_pruner.h: http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.h@66 PS1, Line 66: ranges nit: num_ranges ? http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.h@99 PS1, Line 99: std::vector<std::pair< : std::pair<std::string, std::string>, : std::vector<std::tuple<std::string, std::string>>>> > nit: this might be easier to parse as: +1 I'm also curious why both tuple and pair types is used? Any significance to use std::pair instead of std::tuple for particular types? http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc File src/kudu/common/partition_pruner.cc: http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@518 PS1, Line 518: NumRangesRemainingForTests() != 0; nit: this looks a bit strange to re-use test-only method as a base for regular, non-test-only API http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@523 PS1, Line 523: partition_key_ranges_.size() - 1 Could it happen that partition_key_ranges_ is empty? Maybe, add corresponding CHECK() here? http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@524 PS1, Line 524: partition_key_ranges_[last_range] nit: BTW, there is std::vector::back() method http://gerrit.cloudera.org:8080/#/c/17643/1/src/kudu/common/partition_pruner.cc@550 PS1, Line 550: partition.range_key_start(); Would it be possible to use partition_key_start() without wrapping it into a Slice? -- To view, visit http://gerrit.cloudera.org:8080/17643 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I05c37495430f61a2c6f6012c72251138aee465b7 Gerrit-Change-Number: 17643 Gerrit-PatchSet: 1 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 12 Jul 2021 15:05:38 +0000 Gerrit-HasComments: Yes
