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
Date Wed, 05 Apr 2017 04:40:34 GMT
Zach Amsden has posted comments on this change.

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


Patch Set 15:

(5 comments)

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 968:       BitSet candidates = new BitSet(conjuncts.size());
Alex, thanks for the BitSet suggest - this worked out far cleaner than any other solution.


Line 985:           if (index < 0) continue;
This can be Preconditions.checkState(index >= 0) now.  In the prior form of the code, changed
was a set of exprs, so we could have multiple rewrites of the same index.  With a BitSet,
this is no longer true.


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 custom cluster or
ee test that validates this?


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 implicit casts is legal.
 The only case I can think of where this could be a problem is if the cast is applied to the
constant, which results in truncation, but I don't believe we can create those kind of casts
implicitly.  Consider:

select * from functional.alltypes a
where a.tinyint_col = cast(10000 as tinyint) and a.tinyint_col CONDITION cast(10000 as tinyint)

With our current integer casting situation, I think we truncate instead of convert to NULL,
so this results in tinyint_col having a value, and since the cast can be evaluated as a literal,
we may perform propagation.  No matter how I try to break this expression, though, it seems
to always evaluate exactly the same as our truncation rule, so I think this is a legal transformation.


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 394: 02:HASH JOIN [INNER JOIN]
This is a very sad plan.


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