[kudu-CR] KUDU-1643 Prune hash partitions based on IN-list predicates

["Dan Burkert (Code Review)" <ge...@cloudera.org>]

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 <ho...@gmail.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Haijie Hong <ho...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes