Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19794 )
Change subject: [cpp-client] KUDU-3455 Reduce space complexity and speed up hash partition pruning for in-list predicate ...................................................................... Patch Set 7: (11 comments) http://gerrit.cloudera.org:8080/#/c/19794/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19794/7//COMMIT_MSG@9 PS7, Line 9: https://gerrit.cloudera.org/c/19568/ It would be better to use the related commit hash i.e. b69dbeb instead, then it would be more convenient to show the info by using "git log b69dbeb". http://gerrit.cloudera.org:8080/#/c/19794/7//COMMIT_MSG@16 PS7, Line 16: https://gerrit.cloudera.org/c/19568/ Same. http://gerrit.cloudera.org:8080/#/c/19794/7//COMMIT_MSG@21 PS7, Line 21: columns nit: keys http://gerrit.cloudera.org:8080/#/c/19794/7//COMMIT_MSG@21 PS7, Line 21: The nit: the http://gerrit.cloudera.org:8080/#/c/19794/7//COMMIT_MSG@22 PS7, Line 22: bigger nit: longger? http://gerrit.cloudera.org:8080/#/c/19794/7//COMMIT_MSG@24 PS7, Line 24: Using nit: using http://gerrit.cloudera.org:8080/#/c/19794/7/src/kudu/common/partition_pruner-test.cc File src/kudu/common/partition_pruner-test.cc: http://gerrit.cloudera.org:8080/#/c/19794/7/src/kudu/common/partition_pruner-test.cc@97 PS7, Line 97: std::vector nit: vector http://gerrit.cloudera.org:8080/#/c/19794/7/src/kudu/common/partition_pruner.cc File src/kudu/common/partition_pruner.cc: http://gerrit.cloudera.org:8080/#/c/19794/7/src/kudu/common/partition_pruner.cc@186 PS7, Line 186: vector<bool> hash_bucket_bitset(hash_dimension.num_buckets, false); nit: Move this line to L207 where is closer to the place it's used ? http://gerrit.cloudera.org:8080/#/c/19794/7/src/kudu/common/partition_pruner.cc@217 PS7, Line 217: bool immediately_stop = true; nit: adds DCHECK to check predicate_values_picked and hash_bucket_bitset will not be nullptr? http://gerrit.cloudera.org:8080/#/c/19794/7/src/kudu/common/partition_pruner.cc@219 PS7, Line 219: immediately_stop &= b; : } : if (immediately_stop) { : return; : } Why not return immediately once find a true value in the loop? http://gerrit.cloudera.org:8080/#/c/19794/7/src/kudu/common/partition_pruner.cc@225 PS7, Line 225: CHECK_EQ(hash_dimension.column_ids.size(), predicate_values_list.size()); How about using DCHECK_EQ which is take effect only in DEBUG version? The release version which is usually used in product environment will not be effected. -- To view, visit http://gerrit.cloudera.org:8080/19794 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie4bea5c10b4ac2c62b85625fe9d2a33ceb4fb2e9 Gerrit-Change-Number: 19794 Gerrit-PatchSet: 7 Gerrit-Owner: Yuqi Du <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Tue, 16 May 2023 15:46:39 +0000 Gerrit-HasComments: Yes
