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: Generic constant propagation in planner
Date Fri, 17 Mar 2017 02:59:31 GMT
Zach Amsden has posted comments on this change.

Change subject: IMPALA-5003: Generic constant propagation in planner
......................................................................


Patch Set 3:

(17 comments)

No longer a draft.  Now with tests!

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

Line 79:     rewriter_ = ExprRewriter.createExprRewriter(
> I think we don't actually need the rewriter_ as a member, and instead we ca
I can incorporate this next round if you want, but this diff didn't introduce it as a member.


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

Line 2777:                       + " to " + slotIds.second.toString());
> I'm not even sure this message helps at all. Remove?
Done


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

Line 44: public final class ConstantPropagator {
> brief class comment
Done


Line 51:   //public static ConstantPropagator INSTANCE = new ConstantPropogator();
> remove
Done


PS3, Line 54:   
> comment on thrown exception and state of conjuncts in case of error
fixed


Line 56:   public static boolean propagateConstants(List<Expr> conjuncts, Analyzer analyzer)
> make private, we can still make it public later when the need arises, but f
Done


Line 63:     while (changed) {
> brief comment, maybe with a simple example to show why we need to do this r
Done


Line 80:             try {
> no need for try/catch if we're just going to throw e anyway
Done


Line 87:           output.set(i, rewritten);
> Seems cleaner to only do this when there was a change. That's how our expr 
Done


Line 90:       if (changed) Collections.copy(conjuncts, output);
> Move this outside of the loop. You can iterate over output in L65
Done


Line 95:   // Propogates constants, removes duplicates and optionally performs expr rewriting
> Mention that errors in the rewrite are logged, but ignored, and the 'conjun
Done


Line 96:   public static void optimizeConjuncts(List<Expr> conjuncts, Analyzer analyzer,
> instead of creating a class, why not just move this as static functions int
Expr is already a pretty long class and is more of a utility classes for expr than a policy
or implementation class for any optimizations.  I didn't want aggressive optimization specific
functions to start infecting Expr.

I also thought maybe we could try optimizing (SR1 <= K, SR2 <= SR1 -> SR2 <= K)
(and >= as well, this propagation is valid for any total ordering, but not >,< )
in the future.

If we do that, this class would probably more warrant standing on its own.  Doing so required
more machinery in Expr matching than we currently have so I didn't try it in this diff.


Line 101:         for (int i = 0; i < conjuncts.size(); ++i) {
> rewriter.rewriteList()
Done


Line 107:     catch (AnalysisException e) {
> goes after the } in L106
Done


Line 108:       LOG.error("Not able to analyze after rewrite: " + e.toString());
> LOG.warn and print the conjuncts in this message
Done


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

Line 1217:     ConstantPropagator.optimizeConjuncts(conjuncts, analyzer,
> To clean up this retrieval of enable_expr_rewrites, consider adding an crea
Let me see if that comes out cleaner.


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

Line 48:   public enum RuleSet {
> I see what you are doing, but I don't think we need to be quite as general 
Agreed.  Reduced this to two sets.


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