Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/9223 )
Change subject: IMPALA-6392: Explain format for parquet predicate statistics should be consistent with predicates ...................................................................... Patch Set 2: (9 comments) Thanks for working on this! Do you have a username on JIRA so that I can assign this (and the other ones you're working on) to you? http://gerrit.cloudera.org:8080/#/c/9223/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9223/2//COMMIT_MSG@7 PS2, Line 7: IMPALA-6392: Explain format for parquet predicate statistics should be : consistent with predicates shorten this to fit on one line http://gerrit.cloudera.org:8080/#/c/9223/2//COMMIT_MSG@10 PS2, Line 10: explain format mention that this only affects EXPLAIN_LEVEL=2+ http://gerrit.cloudera.org:8080/#/c/9223/2//COMMIT_MSG@13 PS2, Line 13: Could you include a couple lines showing what the new format looks like? Also, we like to include a "Testing" section. You can look through other reviews on gerrit to see what this normally looks like, but it would be something like: Testing: - Ran existing planner tests and updated the ones that are affected by this change. http://gerrit.cloudera.org:8080/#/c/9223/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/9223/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@230 PS2, Line 230: extra whitespace http://gerrit.cloudera.org:8080/#/c/9223/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@232 PS2, Line 232: extra whitespace http://gerrit.cloudera.org:8080/#/c/9223/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1099 PS2, Line 1099: private String getDictionaryMinMaxOriginalConjunctsExplainString(String prefix) { Brief comment here, like the one for getDictionaryConjunctsExplainString() below http://gerrit.cloudera.org:8080/#/c/9223/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1107 PS2, Line 1107: prefix This can fit on the previous line (we wrap at 90) You might consider looking into clang-format-diff to find these little issues. We include a .clang-format file http://gerrit.cloudera.org:8080/#/c/9223/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1109 PS2, Line 1109: String alias Since this is only used once, no need to make a variable, just include it inline in the append() http://gerrit.cloudera.org:8080/#/c/9223/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1111 PS2, Line 1111: prefix fits on previous line -- To view, visit http://gerrit.cloudera.org:8080/9223 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia3d55ab6a1ae551867a9f68b3622844102cc854e Gerrit-Change-Number: 9223 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Tue, 06 Feb 2018 18:26:25 +0000 Gerrit-HasComments: Yes
