You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@impala.apache.org by "Zach Amsden (Code Review)" <ge...@cloudera.org> on 2017/03/14 17:12:17 UTC

[Impala-ASF-CR] IMPALA-5003: [DRAFT] Generic constant propagation in planner

Zach Amsden has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/6389

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

IMPALA-5003: [DRAFT] Generic constant propagation in planner

Rather than specialize the constant propagation framework to be specific
to a particular node type, create a generic framework for migrating
constants between conjuncts.  Right now it is still hard coded as being
embedded in HdfsScanNode, but the initial support appears to be working.

Testing: So far, manual.  Plan is to examine query plans once the full
support is working.  Right now, the substitution and folding appears to
work, but this doesn't make it to predicates in the query plan, nor does
it help select partitions, so some piece is definitely missing.

 explain select * from zparted where o_orderkey = 10 and partition_id =
o_orderkey % 100 + o_orderkey;

ConstantPropagator.java:85] Constant substitution from slot 2:
         partition_id = 10 % 100 + 10
ConstantPropagator.java:106] Constant folded result: partition_id = 20

 00:SCAN HDFS [default.zparted]
 partitions=100/100 files=100 size=23.00MB
 predicates: o_orderkey = 10, partition_id = o_orderkey % 100 + o_orderkey

Change-Id: I79750a8edb945effee2a519fa3b8192b77042cb4
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/analysis/ConstantPropagator.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
3 files changed, 121 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/89/6389/1
-- 
To view, visit http://gerrit.cloudera.org:8080/6389
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I79750a8edb945effee2a519fa3b8192b77042cb4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>

[Impala-ASF-CR] IMPALA-5003: [DRAFT] Generic constant propagation in planner

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.

Alex Behm has posted comments on this change.

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


Patch Set 2:

(12 comments)

Looks reasonable.

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

Line 2776:             LOG.trace("value transfer: to " + slotIds.first.toString()
nit: can we flip to/from


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

Line 46:   private static ExprRewriter rewriter;
ExprRewriter is not thread-safe. No good to make it static.


Line 51:     List<ExprRewriteRule> rewrite_rules = Lists.newArrayList();
Java style: rewriteRules


Line 53:     rewrite_rules.add(FoldConstantsRule.INSTANCE);
I think we want to re-run all the expr rewrites after constant propagation.

We could either cram everything in here, or minimize the ConstantPropagator. I'd prefer the latter. Constant folding is not strictly necessary here, but normalization is since your the constant-propagation code relies on it.


Line 63:   public static void propagateConstants(List<Expr> l, Analyzer analyzer)
l -> conjuncts?


Line 65:     if (l == null) return;
why not make this a Precondition


Line 66:     int changes = 1;
boolean changed?


Line 71:         if (expr instanceof BinaryPredicate &&
This is a debatable style thing, but in the rest of the FE code we typically invert such conditions and then 'continue' to avoid deep indentation. So in this case:


Line 84:                 ++changes;
Why not run the rewrites here directly. That way we skip over exprs that have not changed.


Line 85:                 LOG.info("Constant substitution from slot " +
Definitely don't want this at info level.


Line 106:             LOG.info("Constant folded result: " + rewritten.toSql());
Definitely don't want this at info level.


Line 111:       for (int i = 0; i < output.size(); ++i) l.set(i, output.get(i));
Collections.copy()


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5003: Constant propagation in scan nodes

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.

Alex Behm has posted comments on this change.

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

Patch Set 15:

(14 comments)

Code is looking good, will take another pass over the tests

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

Line 923: * Propagates constant expressions of the form <slot ref> = <constant> to
Explain candidate param

Line 924: * other uses of slot ref in all expressions in conjuncts; returns a BitSet with
in all expressions in conjuncts -> in the given conjnucts

Line 942: // Don't rewrite with our own substitution! We may have already processed
Explanation seems more complicated than necessary. Clearly it would be wrong to propagate a constant to the originating binary predicate.

Your function comment is quite accurate "to other uses of slot ref" :)

Line 960: * Returns true if the conjuncts may be true and false if a contradiction has
Shorter: Returns false if a contradiction has been implied, true otherwise.

Line 968: BitSet candidates = new BitSet(conjuncts.size());
> Alex, thanks for the BitSet suggest - this worked out far cleaner than any
Yea, this looks pretty good to me!

Line 980: // applied by the rewriter. We must also re-analyze result exprs to
I think we can remove everything after "We must also re-analyze ..." to keep the comment short.

Also, I think constant propagation should fall under the "enable_expr_rewrites" query option, i.e., if false we should only remove duplicates in here. Otherwise, it's hard to say what that query option means at a high level.

Line 985: if (index < 0) continue;
> This can be Preconditions.checkState(index >= 0) now. In the prior form of
Agree. Makes sense.

Line 988: // Reset Expr to force updating cost
Re-analyze expr to add implicit casts, resolve function signatures, and compute cost

Line 998: // because they can't be evaluated if expr rewriting is turned off.
> I can't see a good way to test this with the fe tests. Should we write a c
You should be able to do this with a PlannerTest. For example, look at PlannerTest.testParquetFiltering() which sets a query option and then runs a .test file.

But the easier solution, imo, is to not propagate constants when expr rewriting is disabled.

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 1266:
> Optimization should still be done in the form of eliminating duplicates, ev
Looks like the constant propagation is applied even when expr rewrites are disabled. Imo, it would be preferable not to propagate constants in that case. Otherwise, it's kind of hard to reason about the meaning of that query option.

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

Line 1273: // Allow constant propagation within conjuncts.
This actually has two purposes:
1. constant propagation
2. expr optimization on fully-resolved conjuncts

http://gerrit.cloudera.org:8080/#/c/6389/15/testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test
File testdata/workloads/functional-planner/queries/PlannerTest/constant-propagation.test:

Line 101: # XXX - this seems correct, but is this a legal transformation?
> I'm fairly convinced at this point that considering slotrefs through implic
I agree that implicit casts on both sides seem ok.

It does not seem fine if there is an explicit cast on the slotref.
In your example, the 'cast(10000 as tinyint)' is constant and will be folded (truncated), so this case seems fine.

http://gerrit.cloudera.org:8080/#/c/6389/15/testdata/workloads/functional-planner/queries/PlannerTest/joins.test
File testdata/workloads/functional-planner/queries/PlannerTest/joins.test:

Line 381: # hbase-hdfs join with scan filtering (bogus)
Can you be more specific than "(bogus)"? What behavior are we testing here?

Line 394: 02:HASH JOIN [INNER JOIN]
> This is a very sad plan.
Agree that this can be improved.

--
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: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5003: Constant propagation in scan conjuncts

Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.

Zach Amsden has posted comments on this change.

Change subject: IMPALA-5003: Constant propagation in scan conjuncts
......................................................................


Patch Set 23:

So this tripped up on an functional.alltypes_datasource test, and the first failure was obvious, but now there is a new failure mode which isn't obvious at all, and unclear how it relates to this change:

[localhost:21000] > select string_col from alltypes_datasource
where string_col = 'VALIDATE_PREDICATES##id NOT_DISTINCT 1 && id DISTINCT_FROM 1 && id NOT_DISTINCT 1'
      and 1 <=> id and 1 IS DISTINCT FROM id and 1 IS NOT DISTINCT FROM id;
Query: select string_col from alltypes_datasource
where string_col = 'VALIDATE_PREDICATES##id NOT_DISTINCT 1 && id DISTINCT_FROM 1 && id NOT_DISTINCT 1'
      and 1 <=> id and 1 IS DISTINCT FROM id and 1 IS NOT DISTINCT FROM id
Query submitted at: 2017-04-26 17:20:43 (Coordinator: http://impala-dev:25000)
ERROR: InternalException: Data source DataSource{name=alltypesdatasource, location=hdfs://localhost:20500/test-warehouse/data-sources/test-data-source.jar, className=org.apache.impala.extdatasource.AllTypesDataSource, apiVersion=V1} returned an error from prepare(): Error in data source (path=/tmp/test-data-source.29405.0.jar, class=org.apache.impala.extdatasource.AllTypesDataSource, version=V1) prepare(): No error message returned by data source. Check the impalad log for more information.

-- 
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: 23
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5003: Constant propagation in scan conjuncts

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.

Alex Behm has posted comments on this change.

Change subject: IMPALA-5003: Constant propagation in scan conjuncts
......................................................................


Patch Set 23: Code-Review+2

Let me know if you need help investigation that last issue.

-- 
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: 23
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No