Andrew Sherman 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: (21 comments) Thanks for review comments http://gerrit.cloudera.org:8080/#/c/11719/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11719/4//COMMIT_MSG@51 PS4, Line 51: "" > This quote seems strange here - is it included in the output? This is what you get when you run 'impala-shell.sh -B' http://gerrit.cloudera.org:8080/#/c/11719/4//COMMIT_MSG@69 PS4, Line 69: 80 > Isn't it 60? yes 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) Thanks 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. Done 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 Clever idea 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 Done 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 Done 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 thi Yes, rewritten SQL is already not always valid SQL. EXISTS queries that are rewritten as semi-joins are not legal SQL. I clarified the comment slightly 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? It looks like it should, but the parsedType is not a ParseNode so I did not propagate ToSqlOptions to there. 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? Just some padding for if lines expand. Maybe this is too ugly. 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? Done 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 I don't really understand what is wrong with param? But done. 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? Done 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. Done http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@772 PS4, Line 772: if (line.contains("Analyzed query:")) { : builder.append(line).append("\n"); : inImplictCasts = true; : } else if (inImplictCasts) { : // Keep copying the query with implicit casts. : // This works because this is the last thing in the header. : builder.append(line).append("\n"); : } > optional: this could be expressed in a more compact way: thanks 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_EXP Thanks 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@83 PS4, Line 83: wrap length for testWrapText() - less than 80 to make test layout nicer > nit: Capital W + . at the end. Done http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java@89 PS4, Line 89: * > nit: extra * Done http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java@127 PS4, Line 127: * Check that code that has been wrapped is correctly formatted > nit: missing . Done http://gerrit.cloudera.org:8080/#/c/11719/4/fe/src/test/java/org/apache/impala/util/PrintUtilsTest.java@128 PS4, Line 128: @pa > rm Done 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? Yes. Though I welcome your opinion. The newlines were already inserted by the toSql() code and I thought it clearer to maintain them. -- 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 <asher...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Thu, 01 Nov 2018 00:37:50 +0000 Gerrit-HasComments: Yes