Bharath Vissapragada 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 18: (9 comments) Oops I forgot to "Publish" the comments. I had them in drafts but then I saw the latest comments and realized http://gerrit.cloudera.org:8080/#/c/10571/18//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10571/18//COMMIT_MSG@14 PS18, Line 14: The LOG.trace : statement that prints the rewritten SQL is also updated to use toSql(true). remove? 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@453 PS18, Line 453: try { How about moving the try-catch much closer to the analyze()? http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@473 PS18, Line 473: if (LOG.isTraceEnabled()) { : LOG.trace("rewrittenStmt: " + analysisResult_.stmt_.toSql(true)); : } Do we still need this? http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@479 PS18, Line 479: Error rewritting SQL statement May be say "Error analyzing the rewritten query"? Rewrite is already successful at this point. http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/DeleteStmt.java File fe/src/main/java/org/apache/impala/analysis/DeleteStmt.java: http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/DeleteStmt.java@72 PS18, Line 72: if (!rewritten) { : if merge? http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/DeleteStmt.java@76 PS18, Line 76: StringBuilder b = new StringBuilder(); If rewritten = true and sqlString_ == null, which means toSql(true) is called before ModifyStmt#analyze() runs, the following code could run into NPEs since the toSql()s could be accessing uninitialized state. Should we handle that case? It looks to me like this problem exists in the current version of toSql() too (without this patch), so it is safe to ignore? Basically the precondition for toSql() is that the Stmt should be analyzed. Is my understanding right? http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@939 PS18, Line 939: if (!rewritten) { : // Return the SQL string before inline-view expression substitution. : if (sqlString_ != null) return sqlString_; : } merge http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/StatementBase.java File fe/src/main/java/org/apache/impala/analysis/StatementBase.java: http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/StatementBase.java@135 PS18, Line 135: If rewritten is true..returns the...if the stmt is rewritten..? http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java: http://gerrit.cloudera.org:8080/#/c/10571/18/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@a100 PS18, Line 100: Isn't the semantics that sqlString_ is populated in analyze(), if so, doesn't it make sense to set this to null after rewrite and before reanalyze()? -- 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: 18 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Fri, 13 Jul 2018 18:35:30 +0000 Gerrit-HasComments: Yes
