impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5003: Constant propagation in scan nodes and inline views
Date Tue, 04 Apr 2017 05:19:56 GMT
Zach Amsden has posted comments on this change.

Change subject: IMPALA-5003: Constant propagation in scan nodes and inline views
......................................................................


Patch Set 11:

(15 comments)

Thanks for the review!

http://gerrit.cloudera.org:8080/#/c/6389/11/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

Line 924:    * Propagates constant expression of the form <slot ref> = <constant>
to
> expressions
Done


Line 931:     for (Expr expr: conjuncts) {
> It's probably not that important to worry about perf, but I think it should
I thought about this.  There are certain problems.

1.  If we want to find contradictions, we need to apply the smap to even <slot reg>
= <constant> exprs, for other slot refs.  This is what produces <5> = <7>,
etc..
2.  It is and this is what I originally wanted to do.  But we need to apply the smap to all
exprs except the original expr itself.  There does not seem to be any machinery to apply an
smap to all conjuncts except for the conjunct from which the smap was built, nor an obvious
and uncomplicated way to build it.
3.  This we should probably do.  I had a mock-up that did this at one point in time but abandoned
it for a simpler design that allowed for subsequent or potentially concurrent optimization.
 Now that complexity has crept in at least another dimension (inequality elision), it should
be more obvious how to do this in a way that doesn't prevent the latter optimization at a
future point in time.


Line 964:   private static boolean collateInequalities(List<Expr> conjuncts, Analyzer
analyzer) {
> Please avoid scope creep and leave this out. Feel free to file a new JIRA f
Will do and I agree.  Will cause some minor test refactoring, but that is not a big problem.


http://gerrit.cloudera.org:8080/#/c/6389/11/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java:

Line 968:    * to construct a ValueRange, 
> whitespace
will fix comment


Line 993:       if (!(comp.getOp() == BinaryPredicate.Operator.GT)  &&
> use !=
Done


Line 996:           !(comp.getOp() == BinaryPredicate.Operator.LE)) continue;
> use {} for multi-line ifs
Done


Line 1134:     Expr.optimizeConjuncts(preds, analyzer);
> I don't think we should add this in this patch because the behavior could b
Indeed, optimizing pre-substituted exprs was a bug (caused test failures due to unassigned
conjuncts which are fixed in the latest revision). (See line 1157 just below - eliminating
duplicates and replacing exprs is not valid here!).


Line 1252:     TupleDescriptor tupleDesc = tblRef.getDesc();
> inline into the only use in L1256
Done


Line 1256:     TupleId id = tupleDesc.getId();
> id -> tid
Done


Line 1266:     if (!Expr.optimizeConjuncts(conjuncts, analyzer)) {
> Only if expr rewrites are enabled?
Optimization should still be done in the form of eliminating duplicates, even if rewrites
are disabled.  It was my intention to make rewrites only happen if they were enabled, if that
is not the case, will fix.


Line 1267:       // Conjuncts implied false; convert to emptySetNode
> EmptySetNode
Done


Line 1270:       return node;
> nice!
:)


http://gerrit.cloudera.org:8080/#/c/6389/11/fe/src/main/java/org/apache/impala/planner/ValueRange.java
File fe/src/main/java/org/apache/impala/planner/ValueRange.java:

Line 107:         !(upperBound_ instanceof LiteralExpr) || !(lowerBound_ instanceof LiteralExpr))
> use {} for multi-line ifs
Done


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

Line 48:   private Expr rewrite(Expr expr, Analyzer analyzer, boolean reanalyze)
> Why are these changes needed? Can we avoid them?
Let's discuss in person the best approach here.  The reason for re-analyze is that the higher
level code has no idea what has been rewritten, so it can only attempt to reanalyze everything.


Line 110:   public void reset() {
> single line
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I79750a8edb945effee2a519fa3b8192b77042cb4
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <zamsden@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message