impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4309: Introduce Expr rewrite phase and supporting classes.
Date Sat, 22 Oct 2016 19:57:42 GMT
Marcel Kornacker has posted comments on this change.

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


Patch Set 5:

(11 comments)

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

Line 355
what was going on with this?


Line 379:         analysisResult_.stmt_.rewriteExprs(rewriter_);
for the between transformation it's not necessary, but in general i'd expect to do this repeatedly
until nothing changes, no?


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

Line 56:           "supported in a between predicate: " + toSqlImpl());
capitalize between


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

Line 59:   protected Expr havingClause_;  // original having clause
why move it?


Line 874:       // Also rewrite exprs in the statements of subqueries.
the problem you have is that the parent/child expr tree doesn't give you access to all exprs
in the tree.

instead of hardwiring this logic here and there, how about introducing a Expr.getContainedExprs()
(or some other name) which returns the children and all other contained exprs.

another approach would be to make SelectStmts exprs (and the where/having/group by clauses
would be children), but that seems fairly involved.


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

Line 69
huh?


http://gerrit.cloudera.org:8080/#/c/4746/5/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
File fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java:

Line 38:     numChanges_ = 0;
should rules really have state?

why can't the driver take care of tracking the changes and do the recursive application of
rules, and the rule itself would only apply to a single Expr (and not its children)? that
would also be far better if we have rules that require more complicated interleaving, otherwise
the rules would need to know about each other. see my comments on the rewriter.


Line 46:     if (expr instanceof BetweenPredicate) {
if (!(expr instanceof ...)) return expr;


http://gerrit.cloudera.org:8080/#/c/4746/5/fe/src/main/java/org/apache/impala/rewrite/ExprRewriteRule.java
File fe/src/main/java/org/apache/impala/rewrite/ExprRewriteRule.java:

Line 40:   public abstract Expr rewrite(Expr expr, Analyzer analyzer) throws AnalysisException;
call it apply then?


http://gerrit.cloudera.org:8080/#/c/4746/5/fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java
File fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java:

Line 31: public class ExprRewriter extends ExprRewriteRule {
what's the point of making it a subclass of ExprRewriteRule?

if this is the driver class, it should have the list of rules as well. also, in the future
we might have rules that require repeated and/or interleaved application (think llvm optimization
phases), and this driver class could then encapsulate the knowledge about correct ordering,
etc.


http://gerrit.cloudera.org:8080/#/c/4746/5/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:

Line 186:     // Do not analyze the stmt to avoid applying rewrites.
why is that bad?


-- 
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: 5
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: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message