impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
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:

(13 comments)

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

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

PS4, Line 13: need
remove


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


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


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!!!


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
predicate


PS4, Line 25: there
remove


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?


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 select stmt?


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 rewritten?


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'. Maybe expand
the comment.


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