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 12: Verified+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/19794/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19794/11//COMMIT_MSG@16 PS11, Line 16: the one used by java > nit: the one used by java client Done http://gerrit.cloudera.org:8080/#/c/19794/11/src/kudu/common/partition_pruner-test.cc File src/kudu/common/partition_pruner-test.cc: http://gerrit.cloudera.org:8080/#/c/19794/11/src/kudu/common/partition_pruner-test.cc@1081 PS11, Line 1081: temp_values.push_back(t_value); : test_case.push_back(reinterpret_cast<const void*>(&temp_values[temp_values.size() - 1])); > Can this be simplified to 'test_case.push_back(reinterpret_cast<const void* No, the address of 't_value' is not the same as the last element of temp_values, and t_value is an auto variable in stack, its contents of this address will be undefined after this for loop. The variable 'temp_values' keep the values and its address in this test. http://gerrit.cloudera.org:8080/#/c/19794/11/src/kudu/common/partition_pruner-test.cc@1091 PS11, Line 1091: for (int i = 0; i < kKeyColumnSize; i++) { : predicates.emplace_back(ColumnPredicate::InList(schema.column(i), &test_cases[i])); : } > Maybe this can be moved to the for loop at L1079? And then the 'test_cases' Can not move it. The purpose of lines L1075-L1087 is generating the test_cases, and L1091-1093 use this cases to generator predicates. http://gerrit.cloudera.org:8080/#/c/19794/11/src/kudu/common/partition_pruner.cc File src/kudu/common/partition_pruner.cc: http://gerrit.cloudera.org:8080/#/c/19794/11/src/kudu/common/partition_pruner.cc@224 PS11, Line 224: // All bits in the bitset have been set, we should keep all partitions for > nit: Need to document this short circuit condition. DONE. Add a comment for this. -- 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: Wed, 07 Jun 2023 02:47:02 +0000 Gerrit-HasComments: Yes
