Hello Aman Sinha, Quanlong Huang, Joe McDonnell, Michael Smith, Impala Public
Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/24208
to look at the new patch set (#9).
Change subject: IMPALA-14903: Calcite planner: Simplify code for string literals
......................................................................
IMPALA-14903: Calcite planner: Simplify code for string literals
By default, Calcite always sets string literals to type "char" whereas
Impala needs string literals to be type "string".
The current code committed works around this by creating a cast around
the column. This complicated the code. For example, A Values RelNode
cannot have a cast, so a Project RelNode had to be created on top of a
Values RelNode to handle the cast.
The simplification involved two changes:
1) In ImpalaSqlValidatorImpl, deriveType sets the type to STRING at
validation time
2) Calcite also creates string literals of type CHAR when doing the
SqlNode to RelNode conversion, so it is also changed in RexBuilder
while it is still in the analysis phase.
Other changes included for this:
- As mentioned above, a Project RelNode is no longer needed for casting on
top of the Values node, so the code has been removed from CoerceNodes
- While there are still some cases where the return type needs to be coerced,
there are several cases where this is no longer needed. In CoerceOperandShuttle,
the return type is no longer changed; the value from the Validator
"inferReturnType"
is now always used.
- Because the "inferReturnType" is always used, a couple of minor changes were
also needed to handle this. While this theoretically should be in its own
commit,
they are small enough that they are included here. The "abs", "first_value" and
"last_value" functions are derived from Impala rather than Calcite now. Also, a
method in ImpalaAggOperator was added to allow the use of "ignore nulls" with
"first_value" and "last_value".
- The RemoveUnraggedCharRexExecutor was created solely for the char casting so
it
has been removed.
- We only want to modify the non-default type during the analysis phase. Once
coercion happens, when the type is being determined. During optimization phase,
it is possible that a cast of a tinyint to an integer gets simplified to an int
literal. There is already code in ImpalaRexBuilder to handle this. However, the
post analysis is now set immediately before coercion, allowing some pre-coercion
Calcite code to have its literals cast in the right way.
- A bug was found for the following query:
select * from
(select 1 a, 2 b union all select 1 a, 2 b) x
inner join
(select 1 a, 3 b union all select 1 a, 2 b) y on x.a = y.a
inner join
(select 1 a, 3 b union all select 1 a, 3 b) z on z.b = y.b;
This bug was uncovered by removing the required Project RelNode above the Values
RelNode. This allowed a Filter RelNode to be the parent of a Values RelNode and
the
reduction was buggy. This has been fixed by CALCITE-7450 in version 1.42. After
the
upgrade, the PROJECT_VALUES_MERGE rule can be added as well.
- Some minor changes to the junit tests.
-- Some costs are slightly different for q8. The Project over the Values has
been
eliminated causing an extra predicate to be added.
-- q47 and q57 have a "rank() is not null" predicate removed. This changed
because the
return type is no longer changed, and the nullability of the rank function is
now set
correctly, and no "is not null" clause is needed.
Change-Id: Id8e61b2555afd81ef52f19431fdd1224d4039c00
---
M
java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java
M
java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceOperandShuttle.java
M
java/calcite-planner/src/main/java/org/apache/impala/calcite/functions/RexLiteralConverter.java
M
java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaAggOperator.java
M
java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaOperatorTable.java
M
java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaRexBuilder.java
M
java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaCoreRules.java
M
java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/ImpalaRexExecutor.java
D
java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/RemoveUnraggedCharCastRexExecutor.java
M
java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java
M
java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteRelNodeConverter.java
M
java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ImpalaSqlValidatorImpl.java
M
java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeCoercionImpl.java
M
testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q08.test
M
testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q47.test
M
testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q49.test
M
testdata/workloads/functional-planner/queries/PlannerTest/calcite_tpcds/tpcds-q57.test
17 files changed, 123 insertions(+), 353 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/08/24208/9
--
To view, visit http://gerrit.cloudera.org:8080/24208
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id8e61b2555afd81ef52f19431fdd1224d4039c00
Gerrit-Change-Number: 24208
Gerrit-PatchSet: 9
Gerrit-Owner: Steve Carlin <[email protected]>
Gerrit-Reviewer: Aman Sinha <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Joe McDonnell <[email protected]>
Gerrit-Reviewer: Michael Smith <[email protected]>
Gerrit-Reviewer: Quanlong Huang <[email protected]>
Gerrit-Reviewer: Steve Carlin <[email protected]>
