impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4309: Introduce Expr rewrite phase and supporting classes.
Date Wed, 19 Oct 2016 22:49:07 GMT
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.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message