Dan Burkert has posted comments on this change. Change subject: KUDU-1643 Prune hash partitions based on IN-list predicates ......................................................................
Patch Set 12: (9 comments) Sorry this took so long, was out on holiday break for a stretch. Will try to be much more responsive going forward. Thanks again for taking this on! http://gerrit.cloudera.org:8080/#/c/5176/12/src/kudu/common/partition_pruner-test.cc File src/kudu/common/partition_pruner-test.cc: Line 660: // a in [0]; We automatically simplify single-valued IN list predicates to an equality predicate, so there is no need to test this case. Could you change the remaining cases which use single value IN lists to use equality instead, just to make it more clear? http://gerrit.cloudera.org:8080/#/c/5176/12/src/kudu/common/partition_pruner.cc File src/kudu/common/partition_pruner.cc: Line 154: // Search all combination of in-list and equality predicates. Move this comment to the header. PS12, Line 156: const PartitionSchema& partition_schema, : const PartitionSchema::HashBucketSchema& hash_bucket_schema, // NOLINT(*) : const Schema& schema, : const ScanSpec& scan_spec To avoid the NOLINT you can add a line break before the first argument and indent to 4 spaces, per the google style guide. PS12, Line 165: FindOrNull Use FindOrDie, since the predicate has already been checked, and the result isn't checked against nullptr. PS12, Line 183: static_cast<size_t>(col_offset) this static cast can be avoided by making the col_offset a size_t to begin with. PS12, Line 312: hash_bucket_bitset I think you need to wrap hash_bucket_bitset in std::move in order to avoid a copy here. http://gerrit.cloudera.org:8080/#/c/5176/12/src/kudu/common/partition_pruner.h File src/kudu/common/partition_pruner.h: PS12, Line 67: SearchHashOfColumnCombination I think a better name for this method is 'PruneHashComponent', since it's being applied to just a single hash component. PS12, Line 67: vector use the fully qualified std::vector in header files. Line 68: const PartitionSchema::HashBucketSchema& hash_bucket_schema, // NOLINT(*) See comment .cc about wrapping and NOLINT. -- To view, visit http://gerrit.cloudera.org:8080/5176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I578ac3c5e6770b13b0bebe01127d7d8263aceb36 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Haijie Hong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Haijie Hong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
