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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
