Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17643 )
Change subject: [pruning] KUDU-2671: Pruning compatible with custom hash schemas. ...................................................................... Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc File src/kudu/common/partition_pruner-test.cc: http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner-test.cc@1490 PS6, Line 1490: } I'm not sure whether I saw testcases to cover situations when scan_spec.CanShortCircuit() in the very beginning of the PartitionPruner::Init() method would return 'true' in case of using custom hash bucket schemas per range partition. Does it make sense to add a few to cover the cases to cover various cases when ScanSpec::CanShortCircuit() returns 'true'? http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc File src/kudu/common/partition_pruner.cc: http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@241 PS6, Line 241: ColumnId nit: could use 'auto' here as well? http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@243 PS6, Line 243: ColumnPredicate *predicate nit: per code style, the asterisk should stick to the type, not variable http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@521 PS6, Line 521: range nit: for better readability, maybe name this iterator 'range_it'? http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@521 PS6, Line 521: partition_key_range.second nit: could establish a reference variable to refer to partition_key_range.second for brevity: auto& key_range = partition_key_range.second; http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@523 PS6, Line 523: range++ nit: prefer using prefix increment operators -- they might be faster because they don't need to store the intermediate result in a temporary variable (you could check the implementation of prefix and postfix operators for STD containers' iterators) http://gerrit.cloudera.org:8080/#/c/17643/6/src/kudu/common/partition_pruner.cc@557 PS6, Line 557: && nit: move '&&' to the previous line -- 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: 6 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: Sat, 31 Jul 2021 02:12:04 +0000 Gerrit-HasComments: Yes
