Alex Behm has posted comments on this change. Change subject: IMPALA-4309: Introduce Expr rewrite phase and supporting classes. ......................................................................
Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java File fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java: PS2, Line 26: becuase > because Done http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java File fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java: Line 29: * Rewrites BetweenPredicates a conjunctive/disjunctive CompoundPredicate. > This sentence has some grammatical issues. Done Line 40: private Expr rewriteImpl(Expr expr, Analyzer analyzer) throws AnalysisException { > Maybe rewriteRecursive? Unless there's an existing convention to use Impl f Done http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/rewrite/ExprRewriteRule.java File fe/src/main/java/org/apache/impala/rewrite/ExprRewriteRule.java: Line 36: * transformed Expr. The returned Expr is analyzed. > Is this called for the top level of each expression tree, or for each subex The caller can decide, but typically you'd call this on the root of an Expr tree, i.e., there is no external visitor that drives the expr-tree traversal and calls this at every node or something like that. I though about the latter approach, but I could easily come up with examples where the visitor approach might complicate/obfuscate certain types of rewrites. Having self-contained rules (without an external visitor) seemed simpler, and sharing the trivial tree traversal code did not seem worth the complication. http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java: Line 60: public AnalysisContext RewritesOk(String stmt, int expectedRewriteCount) { > It would also be helpful to test that the expressions are actually replaced Good point. Added extra validation. Let me know if you had something else (e.g. more visual) in mind. -- 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: 2 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: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
