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 12: (7 comments) http://gerrit.cloudera.org:8080/#/c/19794/12/src/kudu/common/partition_pruner-test.cc File src/kudu/common/partition_pruner-test.cc: http://gerrit.cloudera.org:8080/#/c/19794/12/src/kudu/common/partition_pruner-test.cc@96 PS12, Line 96: Return hash values bitset of these combinations. How about: Return a bitset indicates which hash buckets are selected of these combinations. http://gerrit.cloudera.org:8080/#/c/19794/12/src/kudu/common/partition_pruner-test.cc@193 PS12, Line 193: vector<bool> PartitionPrunerTest::PruneHashComponent( Is it copied from partition_pruner.cc? http://gerrit.cloudera.org:8080/#/c/19794/12/src/kudu/common/partition_pruner-test.cc@984 PS12, Line 984: Check the correctness of new algorithm by comparing it with the old one. How about add some tests before this patch, the tests will pass by the 'old algorithm', then replace the old algorithm by the new one, make sure the tests added before will pass as well. http://gerrit.cloudera.org:8080/#/c/19794/12/src/kudu/common/partition_pruner.cc File src/kudu/common/partition_pruner.cc: http://gerrit.cloudera.org:8080/#/c/19794/12/src/kudu/common/partition_pruner.cc@204 PS12, Line 204: vector<const void*> predicate_values_picked; Could you please add some comments to describe what do 'predicate_values_picked' and 'hash_bucket_bitset' store? http://gerrit.cloudera.org:8080/#/c/19794/12/src/kudu/common/partition_pruner.cc@206 PS12, Line 206: vector<bool> hash_bucket_bitset(hash_dimension.num_buckets, false); nit: rename to 'hash_bucket_selected' ? http://gerrit.cloudera.org:8080/#/c/19794/12/src/kudu/common/partition_pruner.cc@219 PS12, Line 219: immediately_stop How about renaming it closer to what it stands for? Like 'all_hash_bucket_needed' or something like that. http://gerrit.cloudera.org:8080/#/c/19794/12/src/kudu/common/partition_pruner.cc@224 PS12, Line 224: All bits in the bitset have been set, If it has a more meaningful name, I gusee these comments can be omitted? -- 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: 12 Gerrit-Owner: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang <chinazhangyi...@163.com> Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Reviewer: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Comment-Date: Fri, 09 Jun 2023 09:40:34 +0000 Gerrit-HasComments: Yes
