Skip to content

Commit 9539521

Browse files
mihaibudiumihaibudiu
committed
[CALCITE-7646] CorrelateProjectExtractor does not handle nested field accesses cor0.field0.field1
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
1 parent f1d76ab commit 9539521

2 files changed

Lines changed: 113 additions & 5 deletions

File tree

‎core/src/main/java/org/apache/calcite/sql2rel/CorrelateProjectExtractor.java‎

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -88,15 +88,33 @@ public CorrelateProjectExtractor(RelBuilderFactory factory) {
8888
this.builderFactory = factory;
8989
}
9090

91+
/** Returns whether {@code node} is a direct field access on the correlation
92+
* variable with the specified id, such as {@code $cor0.DEPTNO}. A nested
93+
* access such as {@code $cor0.REC.DEPTNO} is not direct. */
94+
private static boolean isDirectFieldAccess(RexNode node, CorrelationId id) {
95+
if (node instanceof RexFieldAccess) {
96+
RexFieldAccess access = (RexFieldAccess) node;
97+
if (access.getReferenceExpr() instanceof RexCorrelVariable) {
98+
RexCorrelVariable var = (RexCorrelVariable) access.getReferenceExpr();
99+
return var.id.equals(id);
100+
}
101+
}
102+
return false;
103+
}
104+
91105
@Override public RelNode visit(LogicalCorrelate correlate) {
92106
RelNode left = correlate.getLeft().accept(this);
93107
RelNode right = correlate.getRight().accept(this);
94108
int oldLeft = left.getRowType().getFieldCount();
95109
// Find the correlated expressions from the right side that can be moved to the left
96110
Set<RexNode> callsWithCorrelationInRight =
97111
findCorrelationDependentCalls(correlate.getCorrelationId(), right);
112+
// Only direct field accesses on the correlation variable, such as
113+
// $cor0.DEPTNO, are left in place. A nested field access, such as
114+
// $cor0.REC.DEPTNO, is extracted
98115
boolean isTrivialCorrelation =
99-
callsWithCorrelationInRight.stream().allMatch(exp -> exp instanceof RexFieldAccess);
116+
callsWithCorrelationInRight.stream()
117+
.allMatch(exp -> isDirectFieldAccess(exp, correlate.getCorrelationId()));
100118
// Early exit condition
101119
if (isTrivialCorrelation) {
102120
if (correlate.getLeft().equals(left) && correlate.getRight().equals(right)) {
@@ -264,8 +282,13 @@ private static boolean isSimpleCorrelatedExpression(RexNode node, CorrelationId
264282
* +(10, $cor0.DEPTNO) -> TRUE
265283
* /(100,+(10, $cor0.DEPTNO)) -> TRUE
266284
* CAST(+(10, $cor0.DEPTNO)):INTEGER NOT NULL -> TRUE
285+
* CASE(IS NOT NULL($cor0.PATH), $cor0.PATH, ARRAY(NULL)) -> TRUE
267286
* +($0, $cor0.DEPTNO) -> FALSE
268287
* }</pre>
288+
*
289+
* <p>A subexpression built only from literals and dynamic parameters, such
290+
* as {@code ARRAY(NULL)} above, is neutral: it neither qualifies nor
291+
* disqualifies the enclosing call.
269292
*/
270293
private static class SimpleCorrelationDetector
271294
extends RexVisitorImpl<@Nullable Boolean> {
@@ -284,15 +307,17 @@ private SimpleCorrelationDetector(CorrelationId corrId) {
284307
return Boolean.FALSE;
285308
}
286309

287-
@Override public Boolean visitCall(RexCall call) {
310+
@Override public @Nullable Boolean visitCall(RexCall call) {
311+
// Constant operands must not disqualify the call
288312
Boolean hasSimpleCorrelation = null;
289313
for (RexNode op : call.operands) {
290314
Boolean b = op.accept(this);
291315
if (b != null) {
292316
hasSimpleCorrelation = hasSimpleCorrelation == null ? b : hasSimpleCorrelation && b;
293317
}
294318
}
295-
return hasSimpleCorrelation == null ? Boolean.FALSE : hasSimpleCorrelation;
319+
// If unsure return null; caller will decide
320+
return hasSimpleCorrelation;
296321
}
297322

298323
@Override public @Nullable Boolean visitFieldAccess(RexFieldAccess fieldAccess) {
@@ -332,8 +357,10 @@ private static RexNode replaceCorrelationsWithInputRef(RexNode exp, RelBuilder b
332357
}
333358

334359
/**
335-
* A visitor traversing row expressions and replacing calls with other expressions according
336-
* to the specified mapping.
360+
* A visitor traversing row expressions and replacing calls and field
361+
* accesses with other expressions according to the specified mapping.
362+
* The mapping is consulted before recursing so that the outermost
363+
* matching expression wins.
337364
*/
338365
private static final class CallReplacer extends RexShuttle {
339366
private final Map<RexNode, RexNode> mapping;
@@ -350,5 +377,14 @@ private static final class CallReplacer extends RexShuttle {
350377
return super.visitCall(oldCall);
351378
}
352379
}
380+
381+
@Override public RexNode visitFieldAccess(RexFieldAccess fieldAccess) {
382+
RexNode replacement = mapping.get(fieldAccess);
383+
if (replacement != null) {
384+
return replacement;
385+
} else {
386+
return super.visitFieldAccess(fieldAccess);
387+
}
388+
}
353389
}
354390
}

‎core/src/test/java/org/apache/calcite/sql2rel/CorrelateProjectExtractorTest.java‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,78 @@ public static Frameworks.ConfigBuilder config() {
8484
assertThat(after, hasTree(planAfter));
8585
}
8686

87+
/** Test case for <a href="https://issues.apache.org/jira/projects/CALCITE/issues/CALCITE-7646">[CALCITE-7646]
88+
* CorrelateProjectExtractor does not handle nested field accesses cor0.field0.field1</a>. */
89+
@Test void testNestedCorrelationFieldAccessInFilter() {
90+
final RelBuilder builder = RelBuilder.create(config().build());
91+
final Holder<@Nullable RexCorrelVariable> v = Holder.empty();
92+
RelNode before = builder.scan("EMP")
93+
.project(
94+
builder.alias(
95+
builder.call(SqlStdOperatorTable.ROW,
96+
builder.field("EMPNO"), builder.field("DEPTNO")), "R"))
97+
.variable(v::set)
98+
.scan("DEPT")
99+
.filter(
100+
builder.equals(builder.field(0),
101+
builder.getRexBuilder().makeFieldAccess(builder.field(v.get(), "R"), 1)))
102+
.correlate(JoinRelType.LEFT, v.get().id, builder.field(2, 0, "R"))
103+
.build();
104+
105+
final String planBefore = ""
106+
+ "LogicalCorrelate(correlation=[$cor0], joinType=[left], requiredColumns=[{0}])\n"
107+
+ " LogicalProject(R=[ROW($0, $7)])\n"
108+
+ " LogicalTableScan(table=[[scott, EMP]])\n"
109+
+ " LogicalFilter(condition=[=($0, $cor0.R.EXPR$1)])\n"
110+
+ " LogicalTableScan(table=[[scott, DEPT]])\n";
111+
assertThat(before, hasTree(planBefore));
112+
113+
RelNode after = before.accept(new CorrelateProjectExtractor(RelFactories.LOGICAL_BUILDER));
114+
final String planAfter = ""
115+
+ "LogicalProject(R=[$0], DEPTNO=[$2], DNAME=[$3], LOC=[$4])\n"
116+
+ " LogicalCorrelate(correlation=[$cor0], joinType=[left], requiredColumns=[{1}])\n"
117+
+ " LogicalProject(R=[ROW($0, $7)], $f1=[ROW($0, $7).EXPR$1])\n"
118+
+ " LogicalTableScan(table=[[scott, EMP]])\n"
119+
+ " LogicalFilter(condition=[=($0, $cor0.$f1)])\n"
120+
+ " LogicalTableScan(table=[[scott, DEPT]])\n";
121+
assertThat(after, hasTree(planAfter));
122+
}
123+
124+
/** Tests that a constant call operand, such as {@code POWER(2, 3)}, does
125+
* not prevent extracting the enclosing correlated call. */
126+
@Test void testCorrelationCallWithConstantCallOperandInFilter() {
127+
final RelBuilder builder = RelBuilder.create(config().build());
128+
final Holder<@Nullable RexCorrelVariable> v = Holder.empty();
129+
RelNode before = builder.scan("EMP")
130+
.variable(v::set)
131+
.scan("DEPT")
132+
.filter(
133+
builder.equals(builder.field(0),
134+
builder.call(SqlStdOperatorTable.PLUS,
135+
builder.call(SqlStdOperatorTable.POWER,
136+
builder.literal(2), builder.literal(3)),
137+
builder.field(v.get(), "DEPTNO"))))
138+
.correlate(JoinRelType.LEFT, v.get().id, builder.field(2, 0, "DEPTNO"))
139+
.build();
140+
141+
final String planBefore = ""
142+
+ "LogicalCorrelate(correlation=[$cor0], joinType=[left], requiredColumns=[{7}])\n"
143+
+ " LogicalTableScan(table=[[scott, EMP]])\n"
144+
+ " LogicalFilter(condition=[=($0, +(POWER(2, 3), $cor0.DEPTNO))])\n"
145+
+ " LogicalTableScan(table=[[scott, DEPT]])\n";
146+
assertThat(before, hasTree(planBefore));
147+
148+
RelNode after = before.accept(new CorrelateProjectExtractor(RelFactories.LOGICAL_BUILDER));
149+
final String planAfter = ""
150+
+ "LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], DEPTNO0=[$9], DNAME=[$10], LOC=[$11])\n"
151+
+ " LogicalCorrelate(correlation=[$cor0], joinType=[left], requiredColumns=[{8}])\n"
152+
+ " LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], $f8=[+(POWER(2, 3), $7)])\n"
153+
+ " LogicalTableScan(table=[[scott, EMP]])\n"
154+
+ " LogicalFilter(condition=[=($0, $cor0.$f8)])\n"
155+
+ " LogicalTableScan(table=[[scott, DEPT]])\n";
156+
assertThat(after, hasTree(planAfter));
157+
}
158+
87159
@Test void testDoubleCorrelationCallOverVariableInFilters() {
88160
final RelBuilder builder = RelBuilder.create(config().build());
89161
final Holder<@Nullable RexCorrelVariable> v = Holder.empty();

0 commit comments

Comments
 (0)