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 23:29:47 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-4309: Introduce Expr rewrite phase and supporting classes.
......................................................................


Patch Set 2:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java:

PS2, Line 68: List<ExprRewriteRule> rules = Lists.newArrayList();
            :     rules.add(BetweenToCompoundRule.INSTANCE)
> I think these lines can be combined into one, Lists.newArrayList(BetweenToC
It doesn't fit into one line, so I think this version is more readable (and same number of
lines). Keep in mind that we'd need to:

Lists.<ExprRewriteRule>newArrayList(BetweenToCompoundRule.INSTANCE)


PS2, Line 327: rewriteSubqueries
> IMO, the new method name is a little misleading given it is not actually pe
Done


http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

PS2, Line 1037:  if (conjunct instanceof BetweenPredicate) {
              :           conjunct = BetweenToCompoundRule.INSTANCE.rewrite(conjunct, this);
              :         }
> Just curious why this has been added. Shouldn't it work even without it?
Added comment.

We need this because analyze() registers predicates with the Analyzer and this function is
called as part of registration. In the initial analysis pass, we haven't rewritten BetweenPredicates
yet.

This is one of the reasons why it's cleaner to also separate conjunct registration from analyze().
I have an abandoned patch for that which I might pick up later.


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:

Line 50:   public void analyze(Analyzer analyzer) throws AnalysisException {
> so nice to read the cleaned-up version :)
haha indeed!


http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/analysis/SelectList.java
File fe/src/main/java/org/apache/impala/analysis/SelectList.java:

Line 88: 
> @Override for consistency
Doesn't override.


http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java:

Line 206:     stmt.whereClause_ = BetweenToCompoundRule.INSTANCE.rewrite(stmt.whereClause_,
analyzer);
> long line.
Done


http://gerrit.cloudera.org:8080/#/c/4746/2/fe/src/main/java/org/apache/impala/analysis/TableRef.java
File fe/src/main/java/org/apache/impala/analysis/TableRef.java:

Line 513: 
> @Override for consistency.
Doesn't override


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 36:     numChanges_ = 0;
> This doesn't seem to be thread safe. Given we are using a static INSTANCE, 
Good catch, this was a straight-up bug. Fixed.


-- 
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: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message