Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-4309: Introduce Expr rewrite phase and supporting classes. ......................................................................
Patch Set 4: (13 comments) Flushing out some comments. Haven't looked at tests yet. http://gerrit.cloudera.org:8080/#/c/4746/4//COMMIT_MSG Commit Message: PS4, Line 13: need remove http://gerrit.cloudera.org:8080/#/c/4746/4/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: PS4, Line 378: rewriter_.reset(); : analysisResult_.stmt_.rewriteExprs(rewriter_); : reAnalyze = rewriter_.changed(); I haven't looked at the rewriter_ class yet, but it's not intuitive to me that the rewriter maintains state to indicate that it modified whatever entity it was applied on (Exprs, subqueries, etc). It should either return the modified entity or the entity itself should track modification. Again, I haven't gone through all the classes, just wanted to add a note. PS4, Line 383: StmtRewriter.rewrite(analysisResult_); This is the dual of L379 :). Here, the rewriter applies rewrite() on the analysisResult_ whereas in L379 the analysisResult_ calls rewrite by taking the rewriter as a param. Maybe at some point we need to make it consistent. http://gerrit.cloudera.org:8080/#/c/4746/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: PS4, Line 60: rewrite Maybe "rewriter"? Seeing a verb here is kind of weird :) PS4, Line 1037: if (conjunct instanceof BetweenPredicate) { : // We might be in the first analysis pass, so the conjunct may not have been : // rewritten yet. : conjunct = new BetweenToCompoundRule().rewrite(conjunct, this); : } Maybe comment that this is needed for the EvalPredicate because the BE can't handle the original between predicate. http://gerrit.cloudera.org:8080/#/c/4746/4/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java File fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java: PS4, Line 37: : : : : : : So nice!!! PS4, Line 152: : : : : : I think it's best to add a comment on why BetweenPredicate doesn't need reset any more. PS4, Line 24: predicates predicate PS4, Line 25: there remove http://gerrit.cloudera.org:8080/#/c/4746/4/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java: PS4, Line 220: createStmt_.reset(); We reset a createTableStmt? http://gerrit.cloudera.org:8080/#/c/4746/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java: PS4, Line 206: stmt.whereClause_ = new BetweenToCompoundRule().rewrite(stmt.whereClause_, analyzer); Shouldn't this happen already by the rewriteExprs() call at the top-level select stmt? http://gerrit.cloudera.org:8080/#/c/4746/4/fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java File fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java: PS4, Line 118: Expr clonedConjunct = betweenToCompoundRule_.rewrite(conjunct.clone(), analyzer); Maybe I am missing something, but shouldn't the conjunct already be rewritten? http://gerrit.cloudera.org:8080/#/c/4746/4/fe/src/main/java/org/apache/impala/rewrite/ExprRewriteRule.java File fe/src/main/java/org/apache/impala/rewrite/ExprRewriteRule.java: PS4, Line 35: returns the : * transformed Expr tree What happens if no changes occurred? Is the return value the same as 'expr'. Maybe expand the comment. -- To view, visit http://gerrit.cloudera.org:8080/4746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2279dc984bcf7742db4fa3b1aa67283ecbb05e6e Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
