impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4309: Introduce Expr rewrite phase and supporting classes.
Date Thu, 20 Oct 2016 07:01:28 GMT
Dimitris Tsirogiannis has posted comments on this change.

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

Patch Set 4:


Flushing out some comments. Haven't looked at tests yet.
Commit Message:

PS4, Line 13: need
File fe/src/main/java/org/apache/impala/analysis/

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.
File fe/src/main/java/org/apache/impala/analysis/

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.
File fe/src/main/java/org/apache/impala/analysis/

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

PS4, Line 25: there
File fe/src/main/java/org/apache/impala/analysis/

PS4, Line 220: createStmt_.reset();
We reset a createTableStmt?
File fe/src/main/java/org/apache/impala/analysis/

PS4, Line 206: stmt.whereClause_ = new BetweenToCompoundRule().rewrite(stmt.whereClause_,
Shouldn't this happen already by the rewriteExprs() call at the top-level select stmt?
File fe/src/main/java/org/apache/impala/planner/

PS4, Line 118: Expr clonedConjunct = betweenToCompoundRule_.rewrite(conjunct.clone(), analyzer);
Maybe I am missing something, but shouldn't the conjunct already be rewritten?
File fe/src/main/java/org/apache/impala/rewrite/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I2279dc984bcf7742db4fa3b1aa67283ecbb05e6e
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message