Csaba Ringhofer 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 1: (4 comments) Nice change, I really missed this feature! http://gerrit.cloudera.org:8080/#/c/11719/1/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/1/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@30 PS1, Line 30: /** : * Returns the SQL string corresponding to this node. : */ Can you explain the relation of the two toSql() functions in the comment? If I understand correctly, then toSql() should always call toSql(ToSqlOptions options) with default option. At the first glance I thought that the parameterless toSql should be removed to avoid possible mistakes, but then I realized the huge number of places where it is called, so now I agree with this solution. http://gerrit.cloudera.org:8080/#/c/11719/1/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/1/fe/src/main/java/org/apache/impala/analysis/ToSqlOptions.java@1 PS1, Line 1: package org.apache.impala.analysis; Apache license is missing (also noticed by the jenkins job). http://gerrit.cloudera.org:8080/#/c/11719/1/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/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@344 PS1, Line 344: TestToSqlWithImplicitCasts Can you add a test (or maybe a few) to check that nested structures like sub queries and expression trees propagate the option? http://gerrit.cloudera.org:8080/#/c/11719/1/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/1/fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java@546 PS1, Line 546: errorLog.append("\nTest file: " + testFile + ".test" : + "\n"); nit: fits to one line -- 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: 1 Gerrit-Owner: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Thu, 18 Oct 2018 16:59:52 +0000 Gerrit-HasComments: Yes
