impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <>
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:

File fe/src/main/java/org/apache/impala/analysis/

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?
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.
File testdata/workloads/functional-planner/queries/PlannerTest/joins.test:

This is a very sad plan.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I79750a8edb945effee2a519fa3b8192b77042cb4
Gerrit-PatchSet: 15
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Zach Amsden <>
Gerrit-HasComments: Yes

View raw message