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 Mon, 24 Oct 2016 21:14:09 GMT
Alex Behm 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?
This was stale code from the first version of subquery rewriting which was later cleaned up.
It's not needed.


Line 379:         analysisResult_.stmt_.rewriteExprs(rewriter_);
> for the between transformation it's not necessary, but in general i'd expec
I was thinking that we'd cross that bridge when our set of rewrite rules becomes complex enough
to warrant that, but easy enough to do now. Done.


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
Done


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


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 ac
Expr.getContainedExprs() does not help because we also need to set the rewritten exprs in
the appropriate clauses of the contained QueryStmt. Sure, we can get the exprs and rewrite
them, but then we don't know where to set them. We could make everything Reference<Expr>.

Let me know if you want to go down the "everything is a ParseNode path" and I'll abandon this
patch. That will be much more involved change and I personally don't think it's worth it.
That change will save us these < 50 LOC to traverse the stmts and I don't see us rewriting
non-Expr ParseNodes with that new infra (we'll likely keep our existing StmtRewriter).


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?
Moved to caller to avoid several reset() and reanalyze() passes.


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?
1. Sure, the driver can keep track of changes instead, but it seems like we'll need excessive
object creation:

for (Rule rule: rules) {
  Expr clone = expr.clone();
  clone = rule.apply(clone);
  changed = clone.equals(expr);
}

I made those changes.

2. I think there are some rewrite rules that are way easier to express if the rule itself
can traverse the entire Expr tree. For example, removing redundant conjuncts or disjuncts
seems a little cumbersome in your proposed approach. Seems like we'd need to have a normalization
pass first to get the redundant exprs next to each other so a single rule application can
detect the redundancy.


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


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


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?
1. No need to make subclass ExprRewriteRule. Done.

2. I don't think we want to hardcode the rules in here because we will want to do different
sets of rewrites in different phases of planning. For example, the BETWEEN rewrite is a logical
rewrite that we can do on the parse "tree". But constant folding will need to be done on the
fully substituted Exprs, i.e., during planning. Of course, we can just brute-force it and
always apply everything but I think it's easier to understand if we have a more nuanced application
of rules.

3. I made the other changed you requested here:
* Keep applying rules until there no more changes.
* Have the driver traverse the expr tree and detect changes.


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?
Added 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: 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