Skip to content

Commit 24cf4f5

Browse files
mihaibudiumihaibudiu
committed
[SQL] Tweak Calcite optimization rules to avoid crashes
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
1 parent fd9b9f0 commit 24cf4f5

File tree

9 files changed

+125
-35
lines changed

9 files changed

+125
-35
lines changed

‎docs/sql/comparisons.md‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,14 +85,19 @@ but always return a Boolean value (sometimes nullable):
8585
<td></td>
8686
</tr>
8787
<tr>
88-
<td><code>&lt;OP&gt; ANY SET</code></td>
88+
<td><code>&lt;OP&gt; ANY _set_or_subquery_</code></td>
8989
<td>check if any of the values in a set compares properly</td>
90-
<td>Example: 10 &lt;= ANY (VALUES 10, 20, 30)</td>
90+
<td>Example: 10 &lt;= ANY (VALUES 10, 20, 30) is true</td>
9191
</tr>
9292
<tr>
93-
<td><code>&lt;OP&gt; ALL SET</code></td>
93+
<td><code>&lt;OP&gt; SOME _set_or_subquery_</code></td>
94+
<td>A synonym for `ANY`</td>
95+
<td>Example: 10 &lt;= SOME (VALUES 10, 20, 30) is true</td>
96+
</tr>
97+
<tr>
98+
<td><code>&lt;OP&gt; ALL _set_or_subquery_</code></td>
9499
<td>check if all the values in a set compare properly</td>
95-
<td>Example: 10 &lt;= ALL (VALUES 10, 20, 30)</td>
100+
<td>Example: 10 &lt;= ALL (VALUES 10, 20, 30) is true</td>
96101
</tr>
97102
<tr>
98103
<td><code>EXISTS query</code></td>

‎sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/CalciteToDBSPCompiler.java‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -417,17 +417,16 @@ void visitCorrelate(LogicalCorrelate correlate) {
417417
DBSPVariablePath dataVar = new DBSPVariablePath(leftElementType.ref());
418418
ExpressionCompiler eComp = new ExpressionCompiler(correlate, dataVar, this.compiler);
419419
DBSPClosureExpression arrayExpression = eComp.compile(projection).closure(dataVar);
420-
DBSPType uncollectElementType = this.convertType(uncollect.getRowType(), false);
420+
DBSPTypeTuple uncollectElementType = this.convertType(uncollect.getRowType(), false).to(DBSPTypeTuple.class);
421421
DBSPType arrayElementType = arrayExpression.getResultType().to(DBSPTypeVec.class).getElementType();
422422
if (arrayElementType.mayBeNull)
423423
// This seems to be a bug in Calcite, we should not need to do this adjustment
424-
uncollectElementType = uncollectElementType.withMayBeNull(true);
424+
uncollectElementType = uncollectElementType.withMayBeNull(true).to(DBSPTypeTuple.class);
425425

426426
DBSPType indexType = null;
427427
if (uncollect.withOrdinality) {
428428
// Index field is always last.
429-
DBSPTypeTuple uncollectTuple = uncollectElementType.to(DBSPTypeTuple.class);
430-
indexType = uncollectTuple.getFieldType(uncollectTuple.size() - 1);
429+
indexType = uncollectElementType.getFieldType(uncollectElementType.size() - 1);
431430
}
432431

433432
// Right projections are applied after uncollect

‎sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/calciteCompiler/CalciteCompiler.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -338,6 +338,9 @@ public CalciteCompiler(CompilerOptions options, IErrorReporter errorReporter) {
338338
planner.setExecutor(RexUtil.EXECUTOR);
339339
this.cluster = RelOptCluster.create(planner, new RexBuilder(this.typeFactory));
340340
this.converterConfig = SqlToRelConverter.config()
341+
// Calcite recommends not using withExpand, but there are no
342+
// rules to decorrelate some queries that withExpand will produce,
343+
// e.g., AggScottTests.testAggregates4
341344
.withExpand(true);
342345
this.validator = null;
343346
this.validateTypes = null;
@@ -651,7 +654,6 @@ RexNode validateLatenessOrWatermark(SqlExtendedColumnDeclaration column,
651654
" - " + value +
652655
" FROM TMP;\n";
653656
Logger.INSTANCE.belowLevel(this, 2)
654-
.append("Submitting for compilation ")
655657
.newline()
656658
.append(sql)
657659
.newline();
@@ -1005,7 +1007,6 @@ RexNode createFunction(SqlCreateFunctionDeclaration decl, SourceFileContents sou
10051007

10061008
String sql = builder.toString();
10071009
Logger.INSTANCE.belowLevel(this, 2)
1008-
.append("Submitting for compilation ")
10091010
.newline()
10101011
.append(sql)
10111012
.newline();

‎sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/frontend/calciteCompiler/CalciteOptimizer.java‎

Lines changed: 45 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import org.apache.calcite.rel.RelNode;
99
import org.apache.calcite.rel.RelVisitor;
1010
import org.apache.calcite.rel.core.Join;
11+
import org.apache.calcite.rel.logical.LogicalCorrelate;
1112
import org.apache.calcite.rel.rules.CoreRules;
1213
import org.apache.calcite.rel.rules.PruneEmptyRules;
1314
import org.apache.calcite.sql2rel.RelDecorrelator;
@@ -134,6 +135,23 @@ void run(RelNode node) {
134135
}
135136
}
136137

138+
/** Helper class to discover whether a query contains correlates */
139+
static class CorrelateFinder extends RelVisitor {
140+
public boolean found = false;
141+
@Override public void visit(
142+
RelNode node, int ordinal,
143+
@org.checkerframework.checker.nullness.qual.Nullable RelNode parent) {
144+
if (node instanceof LogicalCorrelate) {
145+
this.found = true;
146+
}
147+
super.visit(node, ordinal, parent);
148+
}
149+
150+
void run(RelNode node) {
151+
this.go(node);
152+
}
153+
}
154+
137155
void createOptimizer(int level) {
138156
this.addStep(new SimpleOptimizerStep("Constant fold", 2,
139157
CoreRules.COERCE_INPUTS,
@@ -166,10 +184,7 @@ void createOptimizer(int level) {
166184
this.addStep(new SimpleOptimizerStep("Convert to correlates", 1,
167185
CoreRules.FILTER_SUB_QUERY_TO_CORRELATE,
168186
CoreRules.JOIN_SUB_QUERY_TO_CORRELATE,
169-
CoreRules.PROJECT_SUB_QUERY_TO_CORRELATE,
170-
CoreRules.FILTER_CORRELATE,
171-
CoreRules.PROJECT_CORRELATE_TRANSPOSE,
172-
CoreRules.JOIN_TO_CORRELATE
187+
CoreRules.PROJECT_SUB_QUERY_TO_CORRELATE
173188
));
174189
*/
175190

@@ -240,15 +255,32 @@ HepProgram getProgram(RelNode node, int level) {
240255
CoreRules.AGGREGATE_MERGE,
241256
CoreRules.INTERSECT_MERGE);
242257
// this.addStep(merge); -- messes up the shape of uncollect
243-
this.addStep(new SimpleOptimizerStep(
244-
"Move projections", 2,
245-
CoreRules.PROJECT_CORRELATE_TRANSPOSE,
246-
CoreRules.PROJECT_WINDOW_TRANSPOSE,
247-
CoreRules.PROJECT_SET_OP_TRANSPOSE,
248-
CoreRules.FILTER_PROJECT_TRANSPOSE
249-
// Rule is unsound
250-
// CoreRules.PROJECT_JOIN_TRANSPOSE
251-
));
258+
259+
this.addStep(new BaseOptimizerStep("Move projections", level) {
260+
@Override
261+
HepProgram getProgram(RelNode node, int level) {
262+
this.addRules(2,
263+
// Rule is unsound: https://issues.apache.org/jira/browse/CALCITE-6681
264+
// CoreRules.PROJECT_CORRELATE_TRANSPOSE,
265+
CoreRules.PROJECT_WINDOW_TRANSPOSE,
266+
CoreRules.PROJECT_SET_OP_TRANSPOSE,
267+
CoreRules.FILTER_PROJECT_TRANSPOSE
268+
);
269+
/*
270+
// Rule is unsound, hopefully it works if there are no correlates
271+
// Moreover, rule interferes with temporal filters optimization
272+
// Should be made obsolete when we add multijoins.
273+
CorrelateFinder finder = new CorrelateFinder();
274+
finder.run(node);
275+
if (!finder.found) {
276+
this.addRules(level, CoreRules.PROJECT_JOIN_TRANSPOSE);
277+
}
278+
*/
279+
this.builder.addMatchOrder(HepMatchOrder.BOTTOM_UP);
280+
return this.builder.build();
281+
}
282+
});
283+
252284
this.addStep(merge);
253285
this.addStep(new SimpleOptimizerStep("Remove dead code", 0,
254286
CoreRules.AGGREGATE_REMOVE,

‎sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/outer/LowerCircuitVisitor.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import org.dbsp.sqlCompiler.ir.statement.DBSPLetStatement;
3333
import org.dbsp.sqlCompiler.ir.statement.DBSPStatement;
3434
import org.dbsp.sqlCompiler.ir.type.DBSPType;
35+
import org.dbsp.sqlCompiler.ir.type.derived.DBSPTypeTuple;
3536
import org.dbsp.sqlCompiler.ir.type.primitive.DBSPTypeAny;
3637
import org.dbsp.sqlCompiler.ir.type.derived.DBSPTypeRawTuple;
3738
import org.dbsp.sqlCompiler.ir.type.derived.DBSPTypeTupleBase;
@@ -71,7 +72,7 @@ public static DBSPExpression rewriteFlatmap(DBSPFlatmap flatmap) {
7172
// let x0: Vec<i32> = x.0.clone();
7273
// let x1: x.1.clone();
7374
DBSPExpression field = rowVar.deref().field(index).applyCloneIfNeeded();
74-
DBSPVariablePath fieldClone = new DBSPVariablePath(field.getType());
75+
DBSPVariablePath fieldClone = field.getType().var();
7576
DBSPLetStatement stat = new DBSPLetStatement(fieldClone.variable, field);
7677
statements.add(stat);
7778
resultColumns.add(fieldClone.applyCloneIfNeeded());
@@ -117,7 +118,13 @@ public static DBSPExpression rewriteFlatmap(DBSPFlatmap flatmap) {
117118
} else {
118119
if (flatmap.ordinalityIndexType != null) {
119120
// e.1, as produced by the iterator
120-
resultColumns.add(e.field(1));
121+
DBSPExpression eField1 = e.field(1);
122+
// If this is a tuple, flatten it.
123+
if (eField1.getType().is(DBSPTypeTuple.class)) {
124+
resultColumns.addAll(DBSPTypeTuple.flatten(eField1));
125+
} else {
126+
resultColumns.add(eField1);
127+
}
121128
} else if (e.getType().is(DBSPTypeTupleBase.class)) {
122129
// Calcite's UNNEST has a strange semantics:
123130
// If e is a tuple type, unpack its fields here

‎sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/ir/expression/DBSPFlatmap.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,12 @@ public IIndentStream toString(IIndentStream builder) {
205205
} else {
206206
if (this.ordinalityIndexType != null) {
207207
// e.1, as produced by the iterator
208-
expressions.add("e.1");
208+
if (collectionElementType.is(DBSPTypeTupleBase.class)) {
209+
for (int i = 0; i < collectionElementType.to(DBSPTypeTupleBase.class).size(); i++)
210+
expressions.add("e.1." + i);
211+
} else {
212+
expressions.add("e.1");
213+
}
209214
} else if (collectionElementType.is(DBSPTypeTupleBase.class)) {
210215
// Calcite's UNNEST has a strange semantics:
211216
// If e is a tuple type, unpack its fields here

‎sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/quidem/AggScottTests.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,24 @@ public void testPairs() {
3030
(14 rows)""");
3131
}
3232

33+
@Test @Ignore("Cannot decorrelate LATERAL join")
34+
public void testLateral() {
35+
this.qs("""
36+
SELECT deptno, ename
37+
FROM
38+
(SELECT DISTINCT deptno FROM emp) t1,
39+
LATERAL (
40+
SELECT ename, sal
41+
FROM emp
42+
WHERE deptno IN (t1.deptno, t1.deptno)
43+
AND deptno = t1.deptno
44+
ORDER BY sal
45+
DESC LIMIT 3);
46+
deptno | ename
47+
----------------
48+
(0 rows)""");
49+
}
50+
3351
@Test
3452
public void testGrouping() {
3553
this.qs("""

‎sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/simple/RegressionTests.java‎

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,34 @@
1515
import org.junit.Test;
1616

1717
public class RegressionTests extends SqlIoTest {
18+
@Test @Ignore("https://issues.apache.org/jira/browse/CALCITE-6681")
19+
public void issueLateral() {
20+
// This triggers https://issues.apache.org/jira/browse/CALCITE-6681
21+
// If we disable PROJECT_CORRELATE_TRANSPOSE, it fails due to the decorrelator
22+
this.compileRustTestCase("""
23+
CREATE TABLE t2 (
24+
a VARCHAR,
25+
ts INT,
26+
x BIGINT
27+
);
28+
29+
CREATE VIEW v AS
30+
WITH t1(a, ts) AS (VALUES('a', 1))
31+
SELECT * FROM t1
32+
LEFT JOIN LATERAL (
33+
SELECT x
34+
FROM t2
35+
WHERE t2.a = t1.a
36+
AND t2.ts <= t1.ts
37+
LIMIT 1
38+
) ON true
39+
LEFT JOIN LATERAL (
40+
SELECT x
41+
FROM t2
42+
WHERE t2.a = t1.a
43+
) ON true;""");
44+
}
45+
1846
@Test
1947
public void issue2639() {
2048
String sql = """
@@ -119,16 +147,11 @@ CREATE TYPE ResourceSpans AS (
119147
r int,
120148
scopeSpans int ARRAY
121149
);
122-
123-
CREATE TABLE t (
124-
resourceSpans ResourceSpans ARRAY
125-
);
150+
CREATE TABLE t (resourceSpans ResourceSpans ARRAY);
126151
127152
CREATE MATERIALIZED VIEW resource_spans AS
128-
SELECT
129-
resourceSpans.scopeSpans
130-
FROM
131-
t, UNNEST(resourceSpans) WITH ORDINALITY as resourceSpans;""";
153+
SELECT resourceSpans.scopeSpans
154+
FROM t, UNNEST(resourceSpans) WITH ORDINALITY as resourceSpans;""";
132155
this.compileRustTestCase(sql);
133156
}
134157

‎sql-to-dbsp-compiler/slt/src/main/java/org/dbsp/sqllogictest/Main.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ public static void rotateTests() throws IOException, ClassNotFoundException {
6262

6363
List<String> list = new ArrayList<>(tests);
6464
list.sort(String::compareTo);
65-
List<String> toRun = list.subList(firstTest, lastTest);
65+
List<String> toRun = list.subList(firstTest, Math.min(lastTest, list.size()));
6666
String[] args = new String[] { "-v", "-x", "-inc", "-e", "hybrid" };
6767

6868
String wd = System.getProperty("user.dir");

0 commit comments

Comments
 (0)