Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19568 )
Change subject: [java] KUDU-3455 Reduce space complexity about pruning hash partitions for in-list predicate ...................................................................... Patch Set 8: (14 comments) Thank you very much for the fix! I did a quick first look, left a few comments. I'll do one more pass soon to make sure I understand it better, but so far looks good to me. http://gerrit.cloudera.org:8080/#/c/19568/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19568/6//COMMIT_MSG@7 PS6, Line 7: complexity about pruning hash partitions complexity of hash partition pruning http://gerrit.cloudera.org:8080/#/c/19568/6//COMMIT_MSG@9 PS6, Line 9: java-client Kudu Java client http://gerrit.cloudera.org:8080/#/c/19568/6//COMMIT_MSG@12 PS6, Line 12: a little nit: drop 'a little' :) http://gerrit.cloudera.org:8080/#/c/19568/6//COMMIT_MSG@15 PS6, Line 15: algorithm like 'deep first search DFS-based algorithm http://gerrit.cloudera.org:8080/#/c/19568/6//COMMIT_MSG@19 PS6, Line 19: pressure experiments What are "pressure experiments"? http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java File java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java: http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java@349 PS8, Line 349: public Is it possible to keep this package-private, not exposing as public? E.g., take a look at shouldPruneForTests() above. http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java@640 PS8, Line 640: public static BitSet pruneHashComponentV2ForTest(Schema schema, : PartitionSchema.HashBucketSchema hashSchema, : Map<String, KuduPredicate> predicates) { : return pruneHashComponentV2(schema, hashSchema, predicates); : } Is it possible to keep this package-private, not exposing as public? E.g., take a look at shouldPruneForTests() above. http://gerrit.cloudera.org:8080/#/c/19568/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java: http://gerrit.cloudera.org:8080/#/c/19568/6/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java@704 PS6, Line 704: CREATE TABLE t Would be great to describe on the essence of the test cases below in a comment: 'carefully' is too vague to reflect on what's being tested. http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java: http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java@889 PS8, Line 889: public void testInListHashPartitionPruningUsingLargeList() throws Exception { It would be great to add a comment to summarize the essence of this test scenario. http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java@967 PS8, Line 967: BitSet newBitset nit: add 'final' for this as well? http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java@984 PS8, Line 984: \ nit: remove the backslash? http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java@985 PS8, Line 985: oom nit: OOM condition http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java@985 PS8, Line 985: use using http://gerrit.cloudera.org:8080/#/c/19568/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestPartitionPruner.java@986 PS8, Line 986: How to test OOM? Leave a 'TODO' at this case. How about: For details on testing for the OOM condition, see the in-line TODO comment in the end this scenario. -- To view, visit http://gerrit.cloudera.org:8080/19568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icd6f213cb705e1b2a001562cc7cebe4164281723 Gerrit-Change-Number: 19568 Gerrit-PatchSet: 8 Gerrit-Owner: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> 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, 31 Mar 2023 05:36:27 +0000 Gerrit-HasComments: Yes
