Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11719 )
Change subject: IMPALA-5821: Add query with implicit casts to extended explain output. ...................................................................... Patch Set 4: (15 comments) overall good cleanup. mostly minor comments from my end. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java File fe/src/main/java/org/apache/impala/analysis/ParseNode.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@32 PS4, Line 32: sql nit: pls use consistent case (SQL, sql, Sql) http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@32 PS4, Line 32: @param pls omit these javadoc annotations. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@39 PS4, Line 39: * This should return the same result as calling toSql(ToSqlOptions.DEFAULT). would be good to do this via a default interface method. pls add a todo for when we fully move to Java 8. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java File fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@25 PS4, Line 25: default, normal "normal" makes sense if you know what is currently done. perhaps "original query without rewrites"? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@38 PS4, Line 38: // This does have the consequence that the sql with implict casts may not pssibly fail nit: spelling http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@38 PS4, Line 38: may not pssibly fail : // to parse if resubmitted as I found this difficult to understand. Did you mean that Sql produced in this mode may not be valid Sql? I'd prefer the comment be merged with the comment block on L35. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/TypeDef.java File fe/src/main/java/org/apache/impala/analysis/TypeDef.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/analysis/TypeDef.java@165 PS4, Line 165: return parsedType_.toSql(); pass down here? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java File fe/src/main/java/org/apache/impala/common/PrintUtils.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java@110 PS4, Line 110: 32 what's this? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/main/java/org/apache/impala/common/PrintUtils.java@115 PS4, Line 115: exiting sp. existing? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@424 PS4, Line 424: @pa remove http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@451 PS4, Line 451: // AnalyzesOk(stmt.toSql(true), ctx); rm? http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java File fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@750 PS4, Line 750: @para remove the comment annotations. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@801 PS4, Line 801: This would also be included if both more direct: Equivalent to enabling INCLUDE_EXPLAIN_HEADER and EXTENDED_EXPLAIN. http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java File fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java: http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java@128 PS4, Line 128: @pa rm http://gerrit.cloudera.org:8080/#/c/11719/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test File testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test: http://gerrit.cloudera.org:8080/#/c/11719/4/testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test@9 PS4, Line 9: -- +straight_join : * were you expecting the hint to print this way? -- To view, visit http://gerrit.cloudera.org:8080/11719 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I55c3bdacc295137f66b2316a912fc347da30d6b0 Gerrit-Change-Number: 11719 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Sherman <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Wed, 31 Oct 2018 21:30:33 +0000 Gerrit-HasComments: Yes
