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 Fri, 21 Oct 2016 21:40:55 GMT
Alex Behm has posted comments on this change.

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


Patch Set 4:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/4746/4//COMMIT_MSG
Commit Message:

PS4, Line 13: need
> remove
Done


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 t
Don't really agree.

1. How would the containing statement know if an expr was rewritten? We'd have to do a beforeExpr.equals(afterExpr)
for every expr we rewrite. That seems weird because the rewriter knows when it changed something.

2. You suggest that the entity itself (the Expr or the statement?) should track modifications.
How would that work exactly?

3. The modifications are in-place so I don't see a point in returning the modified stmt (which
is exactly the same one as before).


PS4, Line 383: StmtRewriter.rewrite(analysisResult_);
> This is the dual of L379 :). Here, the rewriter applies rewrite() on the an
I don't think uniformity makes sense here because these are two very different use cases.

The StmtRewriter actually modifies clauses at the statement level (adding new TablRefs, SelectListItems,
etc.)

However, the new ExprRewriter takes a single ExprTree and transforms it into another ExprTree.
We could maintain the same API, i.e. use ExprRewriter.rewrite(AnalysisResult) but that would
look something like this:

SelectStmt stmt...

stmt.setWhereClause(rewrite(stmt.getWhereClause());
stmt.setHavingClause(rewrite(stmt.getHavingClause());

The set(get()) pattern just seemed weird to me, but I'm happy to reconsider if you feel strongly.


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 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'
Done


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!!!
Yes, finally!


PS4, Line 152: 
             : 
             : 
             : 
             : 
             : 
> I think it's best to add a comment on why BetweenPredicate doesn't need res
Why? It's just like any other Expr where super.reset() is perfectly sufficient.


PS4, Line 24: predicates
> predicate
Done


PS4, Line 25: there
> remove
Done


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?
Removed.


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 s
Yes, we can remove it if you prefer, but then the rewrites cease to be independent (if we
decide to move things around).


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 rewritt
In practice, yes, but I don't think we should bake that assumption into this prunePartitions()
API. For example, in Amos' patch we'll use the PartitionPruner before rewriting (and reorganizing
that would be awkward).


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'
Done


-- 
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.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message