Yuqi Du 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 13: Verified+1 (7 comments) Thanks for your advices, I fixed them. 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 a bitset indicates which hash buckets are > How about: Done http://gerrit.cloudera.org:8080/#/c/19794/12/src/kudu/common/partition_pruner-test.cc@193 PS12, Line 193: // The old algorithm. It moved from 'partition_pruner.cc'. > Is it copied from partition_pruner.cc? Yes. http://gerrit.cloudera.org:8080/#/c/19794/12/src/kudu/common/partition_pruner-test.cc@984 PS12, Line 984: generate some in-list predicates for these key columns. The old algorith > How about add some tests before this patch, the tests will pass by the 'old It seems more confuse. I add some comments using other words. 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: // A combination of the predicate values is selected to compute the hash buckets. > Could you please add some comments to describe what do 'predicate_values_pi Done http://gerrit.cloudera.org:8080/#/c/19794/12/src/kudu/common/partition_pruner.cc@206 PS12, Line 206: predicate_values_selected.reserve(hash_dimension.column_ids.size()); > nit: rename to 'hash_bucket_selected' ? Done http://gerrit.cloudera.org:8080/#/c/19794/12/src/kudu/common/partition_pruner.cc@219 PS12, Line 219: > How about renaming it closer to what it stands for? Like 'all_hash_bucket_n Done http://gerrit.cloudera.org:8080/#/c/19794/12/src/kudu/common/partition_pruner.cc@224 PS12, Line 224: all_hash_bucket_needed = true; > If it has a more meaningful name, I gusee these comments can be omitted? Removed. -- 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: 13 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: Tue, 13 Jun 2023 08:55:57 +0000 Gerrit-HasComments: Yes
