You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Andrew Sherman (Code Review)" <ge...@cloudera.org> on 2018/11/01 00:37:50 UTC
[Impala-ASF-CR] IMPALA-5821: Add query with implicit casts to extended explain output.
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 <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 01 Nov 2018 00:37:50 +0000
Gerrit-HasComments: Yes