zabetak commented on code in PR #5086:
URL: https://github.com/apache/calcite/pull/5086#discussion_r3580063369
##########
core/src/test/resources/sql/struct.iq:
##########
@@ -337,4 +337,193 @@ select row(emp.*, dept.*).deptno0 from emp join dept on
emp.deptno = dept.deptno
!ok
+# 4 test cases for [CALCITE-7646] CorrelateProjectExtractor
+# does not handle nested field accesses cor0.field0.field1.
+
+# All queries use LATERAL, which converts directly to a Correlate.
Review Comment:
Since the queries make use of lateral they fit better in the `lateral.iq`
file.
##########
core/src/test/resources/sql/struct.iq:
##########
@@ -337,4 +337,193 @@ select row(emp.*, dept.*).deptno0 from emp join dept on
emp.deptno = dept.deptno
!ok
+# 4 test cases for [CALCITE-7646] CorrelateProjectExtractor
+# does not handle nested field accesses cor0.field0.field1.
+
+# All queries use LATERAL, which converts directly to a Correlate.
+# The results were validated on Postgres
+
+!use scott
+
+# Before the fix planning failes with
+# "Mappings$NoElementException: source #0 has no target in mapping".
+select t.dd, t.x
+from dept d,
+lateral (select d.deptno as dd, u.x
+ from unnest(array[d.deptno + 100]) as u(x)) as t
+where d.dname = 'SALES';
++----+-----+
+| DD | X |
++----+-----+
+| 30 | 130 |
++----+-----+
+(1 row)
+
+!ok
+!if (use_old_decorr) {
+EnumerableCalc(expr#0..3=[{inputs}], DD=[$t2], X=[$t3])
+ EnumerableCorrelate(correlation=[$cor1], joinType=[inner],
requiredColumns=[{0, 1}])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=[100], expr#4=[+($t0, $t3)],
expr#5=[ARRAY($t4)], expr#6=['SALES':VARCHAR(14)], expr#7=[=($t1, $t6)],
DEPTNO=[$t0], $f3=[$t5], $condition=[$t7])
+ EnumerableTableScan(table=[[scott, DEPT]])
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor1], expr#2=[$t1.DEPTNO],
DD=[$t2], X=[$t0])
+ EnumerableUncollect
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor1], expr#2=[$t1.$f3],
EXPR$0=[$t2])
+ EnumerableValues(tuples=[[{ 0 }]])
+!plan
+!}
+!if (use_new_decorr) {
+EnumerableCalc(expr#0..3=[{inputs}], expr#4=['SALES':VARCHAR(14)],
expr#5=[=($t1, $t4)], DD=[$t2], X=[$t3], $condition=[$t5])
+ EnumerableCorrelate(correlation=[$cor1], joinType=[inner],
requiredColumns=[{0}])
+ EnumerableCalc(expr#0..2=[{inputs}], proj#0..1=[{exprs}])
+ EnumerableTableScan(table=[[scott, DEPT]])
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor1], expr#2=[$t1.DEPTNO],
DD=[$t2], X=[$t0])
+ EnumerableUncollect
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor1], expr#2=[$t1.DEPTNO],
expr#3=[100], expr#4=[+($t2, $t3)], expr#5=[ARRAY($t4)], EXPR$0=[$t5])
+ EnumerableValues(tuples=[[{ 0 }]])
+!plan
+!}
Review Comment:
The new decorrelator is not using the `CorrelateProjectExtractor` so to some
extend this test is unrelated to the fix and somewhat redundant. Is there
something specific that we want to check in the plan?
##########
core/src/test/resources/sql/struct.iq:
##########
@@ -337,4 +337,193 @@ select row(emp.*, dept.*).deptno0 from emp join dept on
emp.deptno = dept.deptno
!ok
+# 4 test cases for [CALCITE-7646] CorrelateProjectExtractor
+# does not handle nested field accesses cor0.field0.field1.
+
+# All queries use LATERAL, which converts directly to a Correlate.
+# The results were validated on Postgres
+
+!use scott
+
+# Before the fix planning failes with
+# "Mappings$NoElementException: source #0 has no target in mapping".
Review Comment:
The problem/failure (along with stacktrace information) are much more useful
in the Jira description than in comments in the code. Users encountering a
problem will first search on Google/Jira before digging into the code.
For reviewers its also easier to understand the context of the work by
reading the problem description first.
##########
core/src/test/resources/sql/struct.iq:
##########
@@ -337,4 +337,193 @@ select row(emp.*, dept.*).deptno0 from emp join dept on
emp.deptno = dept.deptno
!ok
+# 4 test cases for [CALCITE-7646] CorrelateProjectExtractor
+# does not handle nested field accesses cor0.field0.field1.
+
+# All queries use LATERAL, which converts directly to a Correlate.
+# The results were validated on Postgres
+
+!use scott
+
+# Before the fix planning failes with
+# "Mappings$NoElementException: source #0 has no target in mapping".
+select t.dd, t.x
+from dept d,
+lateral (select d.deptno as dd, u.x
+ from unnest(array[d.deptno + 100]) as u(x)) as t
+where d.dname = 'SALES';
++----+-----+
+| DD | X |
++----+-----+
+| 30 | 130 |
++----+-----+
+(1 row)
+
+!ok
+!if (use_old_decorr) {
+EnumerableCalc(expr#0..3=[{inputs}], DD=[$t2], X=[$t3])
+ EnumerableCorrelate(correlation=[$cor1], joinType=[inner],
requiredColumns=[{0, 1}])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=[100], expr#4=[+($t0, $t3)],
expr#5=[ARRAY($t4)], expr#6=['SALES':VARCHAR(14)], expr#7=[=($t1, $t6)],
DEPTNO=[$t0], $f3=[$t5], $condition=[$t7])
+ EnumerableTableScan(table=[[scott, DEPT]])
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor1], expr#2=[$t1.DEPTNO],
DD=[$t2], X=[$t0])
+ EnumerableUncollect
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor1], expr#2=[$t1.$f3],
EXPR$0=[$t2])
+ EnumerableValues(tuples=[[{ 0 }]])
+!plan
+!}
+!if (use_new_decorr) {
+EnumerableCalc(expr#0..3=[{inputs}], expr#4=['SALES':VARCHAR(14)],
expr#5=[=($t1, $t4)], DD=[$t2], X=[$t3], $condition=[$t5])
+ EnumerableCorrelate(correlation=[$cor1], joinType=[inner],
requiredColumns=[{0}])
+ EnumerableCalc(expr#0..2=[{inputs}], proj#0..1=[{exprs}])
+ EnumerableTableScan(table=[[scott, DEPT]])
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor1], expr#2=[$t1.DEPTNO],
DD=[$t2], X=[$t0])
+ EnumerableUncollect
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor1], expr#2=[$t1.DEPTNO],
expr#3=[100], expr#4=[+($t2, $t3)], expr#5=[ARRAY($t4)], EXPR$0=[$t5])
+ EnumerableValues(tuples=[[{ 0 }]])
+!plan
+!}
+
+select t.dd, t.dd1, t.x
+from dept d,
+lateral (select d.deptno as dd, d.deptno + 1 as dd1, u.x
+ from unnest(array[1, 2]) as u(x)) as t
+where d.dname = 'SALES';
++----+-----+---+
+| DD | DD1 | X |
++----+-----+---+
+| 30 | 31 | 1 |
+| 30 | 31 | 2 |
++----+-----+---+
+(2 rows)
+
+!ok
+!if (use_old_decorr) {
+EnumerableCalc(expr#0..4=[{inputs}], proj#0..2=[{exprs}])
+ EnumerableHashJoin(condition=[AND(=($3, $5), =($4, $6))], joinType=[semi])
+ EnumerableCalc(expr#0..2=[{inputs}], proj#0..2=[{exprs}], DEPTNO=[$t0],
$f3=[$t1])
+ EnumerableNestedLoopJoin(condition=[true], joinType=[inner])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=[1], expr#4=[+($t0, $t3)],
expr#5=['SALES':VARCHAR(14)], expr#6=[=($t1, $t5)], DEPTNO=[$t0], $f3=[$t4],
$condition=[$t6])
+ EnumerableTableScan(table=[[scott, DEPT]])
+ EnumerableUncollect
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[1], expr#2=[2],
expr#3=[ARRAY($t1, $t2)], EXPR$0=[$t3])
+ EnumerableValues(tuples=[[{ 0 }]])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=[1], expr#4=[+($t0, $t3)],
expr#5=['SALES':VARCHAR(14)], expr#6=[=($t1, $t5)], DEPTNO=[$t0], $f3=[$t4],
$condition=[$t6])
+ EnumerableTableScan(table=[[scott, DEPT]])
+!plan
+!}
+!if (use_new_decorr) {
+EnumerableCalc(expr#0..3=[{inputs}], proj#0..2=[{exprs}])
+ EnumerableHashJoin(condition=[IS NOT DISTINCT FROM($3, $4)], joinType=[semi])
+ EnumerableCalc(expr#0..1=[{inputs}], expr#2=[1], expr#3=[+($t0, $t2)],
DEPTNO=[$t0], $f1=[$t3], EXPR$0=[$t1], DEPTNO0=[$t0])
+ EnumerableNestedLoopJoin(condition=[true], joinType=[inner])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=['SALES':VARCHAR(14)],
expr#4=[=($t1, $t3)], DEPTNO=[$t0], $condition=[$t4])
+ EnumerableTableScan(table=[[scott, DEPT]])
+ EnumerableUncollect
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[1], expr#2=[2],
expr#3=[ARRAY($t1, $t2)], EXPR$0=[$t3])
+ EnumerableValues(tuples=[[{ 0 }]])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=['SALES':VARCHAR(14)],
expr#4=[=($t1, $t3)], DEPTNO=[$t0], $condition=[$t4])
+ EnumerableTableScan(table=[[scott, DEPT]])
+!plan
+!}
+
+# COALESCE(d.path, ARRAY[CAST(NULL AS INTEGER)]) converts to
+# CASE(IS NOT NULL($cor0.PATH), $cor0.PATH, ARRAY(null:INTEGER)). The constant
+# ARRAY(null:INTEGER) operand must not prevent extracting the CASE to the
+# left input of the Correlate operator
+select d.deptno, t.x
+from (select deptno,
+ case when deptno = 10 then array[deptno, deptno + 1] end as path
+ from dept) as d,
+lateral (select * from unnest(coalesce(d.path, array[cast(null as integer)]))
as u(x)) as t
+order by d.deptno, t.x;
++--------+----+
+| DEPTNO | X |
++--------+----+
+| 10 | 10 |
+| 10 | 11 |
+| 20 | |
+| 30 | |
+| 40 | |
++--------+----+
+(5 rows)
+
+!ok
+!if (use_old_decorr) {
+EnumerableSort(sort0=[$0], sort1=[$1], dir0=[ASC], dir1=[ASC])
+ EnumerableCalc(expr#0..2=[{inputs}], DEPTNO=[$t0], X=[$t2])
+ EnumerableCorrelate(correlation=[$cor0], joinType=[inner],
requiredColumns=[{1}])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=[CAST($t0):INTEGER NOT
NULL], expr#4=[10], expr#5=[=($t3, $t4)], expr#6=[1], expr#7=[+($t0, $t6)],
expr#8=[ARRAY($t3, $t7)], expr#9=[null:INTEGER NOT NULL ARRAY],
expr#10=[CASE($t5, $t8, $t9)], expr#11=[IS NOT NULL($t10)],
expr#12=[CAST($t10):INTEGER NOT NULL ARRAY NOT NULL],
expr#13=[CAST($t12):INTEGER ARRAY NOT NULL], expr#14=[null:INTEGER],
expr#15=[ARRAY($t14)], expr#16=[CASE($t11, $t13, $t15)], DEPTNO=[$t0],
$f2=[$t16])
+ EnumerableTableScan(table=[[scott, DEPT]])
+ EnumerableUncollect
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor0], expr#2=[$t1.$f2],
EXPR$0=[$t2])
+ EnumerableValues(tuples=[[{ 0 }]])
+!plan
+!}
+!if (use_new_decorr) {
+EnumerableSort(sort0=[$0], sort1=[$1], dir0=[ASC], dir1=[ASC])
+ EnumerableCalc(expr#0..2=[{inputs}], DEPTNO=[$t0], X=[$t2])
+ EnumerableCorrelate(correlation=[$cor0], joinType=[inner],
requiredColumns=[{1}])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=[CAST($t0):INTEGER NOT
NULL], expr#4=[10], expr#5=[=($t3, $t4)], expr#6=[1], expr#7=[+($t0, $t6)],
expr#8=[ARRAY($t3, $t7)], expr#9=[null:INTEGER NOT NULL ARRAY],
expr#10=[CASE($t5, $t8, $t9)], DEPTNO=[$t0], PATH=[$t10])
+ EnumerableTableScan(table=[[scott, DEPT]])
+ EnumerableUncollect
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor0], expr#2=[$t1.PATH],
expr#3=[IS NOT NULL($t2)], expr#4=[CAST($t2):INTEGER NOT NULL ARRAY NOT NULL],
expr#5=[CAST($t4):INTEGER ARRAY NOT NULL], expr#6=[null:INTEGER],
expr#7=[ARRAY($t6)], expr#8=[CASE($t3, $t5, $t7)], EXPR$0=[$t8])
+ EnumerableValues(tuples=[[{ 0 }]])
+!plan
+!}
+
+!use bookstore
+
+# Before the fix decorrelation left a dangling
+# correlated expression, and code generation failed with "AssertionError:
+# Correlation variable $cor1 should be defined".
+
+# EnumerableRules.ENUMERABLE_CORRELATE_RULE is removed so that this test
+# fails if the plan cannot be fully decorrelated.
+!set planner-rules "-EnumerableRules.ENUMERABLE_CORRELATE_RULE"
Review Comment:
Nice idea for ensuring that a plan is fully decorrelated although such test
would fit better in RelDecorrelatorTest where it would be more readable.
##########
core/src/test/resources/sql/struct.iq:
##########
@@ -337,4 +337,193 @@ select row(emp.*, dept.*).deptno0 from emp join dept on
emp.deptno = dept.deptno
!ok
+# 4 test cases for [CALCITE-7646] CorrelateProjectExtractor
+# does not handle nested field accesses cor0.field0.field1.
+
+# All queries use LATERAL, which converts directly to a Correlate.
+# The results were validated on Postgres
+
+!use scott
+
+# Before the fix planning failes with
+# "Mappings$NoElementException: source #0 has no target in mapping".
+select t.dd, t.x
+from dept d,
+lateral (select d.deptno as dd, u.x
+ from unnest(array[d.deptno + 100]) as u(x)) as t
+where d.dname = 'SALES';
++----+-----+
+| DD | X |
++----+-----+
+| 30 | 130 |
++----+-----+
+(1 row)
+
+!ok
+!if (use_old_decorr) {
+EnumerableCalc(expr#0..3=[{inputs}], DD=[$t2], X=[$t3])
+ EnumerableCorrelate(correlation=[$cor1], joinType=[inner],
requiredColumns=[{0, 1}])
+ EnumerableCalc(expr#0..2=[{inputs}], expr#3=[100], expr#4=[+($t0, $t3)],
expr#5=[ARRAY($t4)], expr#6=['SALES':VARCHAR(14)], expr#7=[=($t1, $t6)],
DEPTNO=[$t0], $f3=[$t5], $condition=[$t7])
+ EnumerableTableScan(table=[[scott, DEPT]])
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor1], expr#2=[$t1.DEPTNO],
DD=[$t2], X=[$t0])
+ EnumerableUncollect
+ EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor1], expr#2=[$t1.$f3],
EXPR$0=[$t2])
+ EnumerableValues(tuples=[[{ 0 }]])
+!plan
Review Comment:
Apart from the fact that the query runs (does not crash) and produces
correct results what do we want to assert/verify in the query plan?
If we don't really care about the query plan then we should skip it. If
there is something that we need to validate then an extra comment would be
helpful.
##########
core/src/main/java/org/apache/calcite/sql2rel/CorrelateProjectExtractor.java:
##########
@@ -116,27 +134,42 @@ public CorrelateProjectExtractor(RelBuilderFactory
factory) {
// Transform the correlated expression from the right side to an
expression over the left side
builder.push(left);
+ ImmutableBitSet.Builder requiredColumns = ImmutableBitSet.builder();
List<RexNode> callsWithCorrelationOverLeft = new ArrayList<>();
for (RexNode callInRight : callsWithCorrelationInRight) {
-
callsWithCorrelationOverLeft.add(replaceCorrelationsWithInputRef(callInRight,
builder));
+ if (isDirectFieldAccess(callInRight, correlate.getCorrelationId())) {
Review Comment:
How important is to have this condition? I understand that it tries to block
"trivial" correlated expressions from generating redundant column references
but it shouldn't lead to incorrect plans.
##########
core/src/main/java/org/apache/calcite/sql2rel/CorrelateProjectExtractor.java:
##########
@@ -88,15 +88,33 @@ public CorrelateProjectExtractor(RelBuilderFactory factory)
{
this.builderFactory = factory;
}
+ /** Returns whether {@code node} is a direct field access on the correlation
+ * variable with the specified id, such as {@code $cor0.DEPTNO}. A nested
+ * access such as {@code $cor0.REC.DEPTNO} is not direct. */
+ private static boolean isDirectFieldAccess(RexNode node, CorrelationId id) {
+ if (node instanceof RexFieldAccess) {
+ RexFieldAccess access = (RexFieldAccess) node;
+ if (access.getReferenceExpr() instanceof RexCorrelVariable) {
+ RexCorrelVariable var = (RexCorrelVariable) access.getReferenceExpr();
Review Comment:
`var` is a reserved keyword in JDK 10+ so to avoid any potential confusion I
think its worth addressing this minor sonar warning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
