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 6: (18 comments) http://gerrit.cloudera.org:8080/#/c/19794/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19794/3//COMMIT_MSG@9 PS3, Line 9: This patch comes from https://gerrit.cloudera.org/c/19568/. As that patch > This patch comes from https://gerrit.cloudera.org/c/19568/. Done http://gerrit.cloudera.org:8080/#/c/19794/3//COMMIT_MSG@18 PS3, Line 18: Compared with java client, the cpp client is less likely to cause the : OOM condition because it does not keep too many intermediate results. : This optimization has good be > Compared with the java client, the cpp client is less likely to cause the O Done http://gerrit.cloudera.org:8080/#/c/19794/3//COMMIT_MSG@21 PS3, Line 21: olumns, > the number of Done http://gerrit.cloudera.org:8080/#/c/19794/3//COMMIT_MSG@21 PS3, Line 21: ength and > are related to Done http://gerrit.cloudera.org:8080/#/c/19794/3//COMMIT_MSG@22 PS3, Line 22: would be better if > The performance are better Done http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner-test.cc File src/kudu/common/partition_pruner-test.cc: http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner-test.cc@974 PS3, Line 974: // For test cases that will run with in-list predicates using variant length > I don't quite understand this sentence, please reorganize it. Done http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner-test.cc@982 PS3, Line 982: with 200 columns including > nit: with 10 columns. changed to "with 200 columns including 10 key columns" http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner-test.cc@983 PS3, Line 983: // generate some in-list predicates for these key columns. > Check the correctness of the new algorithm by comparing it with the old one Done http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner-test.cc@984 PS3, Line 984: // Check the correctness of new algorithm by comparing it with the old one. > Compare the efficiency of the two algorithms. Done http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner-test.cc@985 PS3, Line 985: // Compare the efficiency of the two algorithms. > nit: Add 'SKIP_IF_SLOW_NOT_ALLOWED()' for the test. Done http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner-test.cc@1023 PS3, Line 1023: on> partitions; > comparing it with the old one. Done http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner-test.cc@1046 PS3, Line 1046: or (int i = 0; i < v1.size(); i++) { > The following logs are used to compare the efficiency of the two algorithms Done http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner-test.cc@1063 PS3, Line 1063: : constexpr const int kTotalCount = 10; > Increase the vector's capacity to kMaxSafeLength * kKeyColumnSize to avoid Done http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner.h File src/kudu/common/partition_pruner.h: http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner.h@94 PS3, Line 94: // Pick all combinations in in-list values and compute their hash buckets, > nit: Please add some description about this newly added method. Done http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner.cc File src/kudu/common/partition_pruner.cc: http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner.cc@182 PS3, Line 182: > nit: Remove this line. Done http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner.cc@216 PS3, Line 216: predicate_valu > nit: What about renaming it to 'predicate_values_picked'? Done http://gerrit.cloudera.org:8080/#/c/19794/3/src/kudu/common/partition_pruner.cc@242 PS3, Line 242: edicate_v > nit: What about using 'emplace_back'? Done http://gerrit.cloudera.org:8080/#/c/19794/4/src/kudu/common/partition_pruner.cc File src/kudu/common/partition_pruner.cc: http://gerrit.cloudera.org:8080/#/c/19794/4/src/kudu/common/partition_pruner.cc@213 PS4, Line 213: void PartitionPruner::ComputeHashBuckets(const Schema& schema, // NOLINT(misc-no-recursion) > warning: function 'ComputeHashBuckets' is within a recursive call chain [mi Ignore it simply. Maybe solve it by convert this function to a non-recursive function -- 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: 6 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: Sat, 06 May 2023 11:28:44 +0000 Gerrit-HasComments: Yes
