Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10571 )
Change subject: IMPALA-7106: Log the original and rewritten SQL when SQL rewrite fails ...................................................................... Patch Set 19: (7 comments) http://gerrit.cloudera.org:8080/#/c/10571/18//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10571/18//COMMIT_MSG@10 PS18, Line 10: SQL string when errors arise or as the result of "SHOW CREATE". When > when errors arise or as the result of "show create" Done http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@474 PS18, Line 474: Rewritten SQL: > use the same label as L480: "Rewritten SQL: " Done http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/TableRef.java File fe/src/main/java/org/apache/impala/analysis/TableRef.java: http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/TableRef.java@575 PS18, Line 575: boolean rewritten > why's this not used? InlinViewRef is a subclass of TableRef and it's used by the overriden InlineViewRef::tableRefToSql(boolean rewritten) in InlineViewRef, which affects the TableRef::toSql(boolean rewritten) http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java: http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@191 PS18, Line 191: , : boolean rewritten > I think we should remove this and keep the original behavior. The intent is The original behavior is still the same when calling ctasStmt.toSql(), see the new ExpRewriterTest.java L300. http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java: http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@714 PS18, Line 714: BETWEEN > why did this change? I thought the only externally visible change will be r Prior to this patch, the error message shows the rewritten SQL instead of the original one, which is incorrect. If you look at L711, the original SQL uses BETWEEN so the error message should technically return BETWEEN as well. http://gerrit.cloudera.org:8080/#/c/10571/18/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/10571/18/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@237 PS18, Line 237: > nit: remove these newlines following the banner Done http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java@347 PS18, Line 347: : : : : > simpler: Assert.assertEquals(stmt.toSql(), stmt.toSql(true)) Done -- To view, visit http://gerrit.cloudera.org:8080/10571 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab58b0cc865135d261dd4a7f72be130f2e7bde53 Gerrit-Change-Number: 10571 Gerrit-PatchSet: 19 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Fri, 13 Jul 2018 18:31:06 +0000 Gerrit-HasComments: Yes
