Skip to content

Commit 2d202fb

Browse files
committed
feat(isthmus)!: remove invalid SetOp to Calcite relation mappings
The Substrait -> Calcite converter mapped MINUS_MULTISET to Calcite Minus(all=true) and INTERSECTION_PRIMARY to Intersect(all=false). These mappings are incorrect: the spec behaviour of these set operations does not match the Calcite relations they were mapped to. They were retained temporarily to let users migrate off of them. Neither MINUS_MULTISET nor INTERSECTION_PRIMARY has an equivalent Calcite relation, so converting a plan containing either to Calcite now throws UnsupportedOperationException. BREAKING CHANGE: Converting a Substrait plan that uses the MINUS_MULTISET or INTERSECTION_PRIMARY set operations to Calcite now throws UnsupportedOperationException instead of producing an incorrect Calcite relation. The supported set-difference operations are MINUS_PRIMARY (Minus all=false) and MINUS_PRIMARY_ALL (Minus all=true); the supported intersection operations are INTERSECTION_MULTISET (Intersect all=false) and INTERSECTION_MULTISET_ALL (Intersect all=true). Closes #303
1 parent a107b72 commit 2d202fb

3 files changed

Lines changed: 22 additions & 7 deletions

File tree

isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -285,10 +285,6 @@ public RelNode visit(Set set, Context context) throws RuntimeException {
285285
input -> {
286286
relBuilder.push(input.accept(this, context));
287287
});
288-
// TODO: MINUS_MULTISET and INTERSECTION_PRIMARY mappings are set to be removed as they do not
289-
// correspond to the Calcite relations they are associated with. They are retained for now
290-
// to enable users to migrate off of them.
291-
// See: https://github.com/substrait-io/substrait-java/issues/303
292288
RelBuilder builder = getRelBuilder(set);
293289
RelNode node = builder.build();
294290
return applyRemap(node, set.getRemap());
@@ -301,9 +297,7 @@ private RelBuilder getRelBuilder(Set set) {
301297
case MINUS_PRIMARY:
302298
return relBuilder.minus(false, numInputs);
303299
case MINUS_PRIMARY_ALL:
304-
case MINUS_MULTISET:
305300
return relBuilder.minus(true, numInputs);
306-
case INTERSECTION_PRIMARY:
307301
case INTERSECTION_MULTISET:
308302
return relBuilder.intersect(false, numInputs);
309303
case INTERSECTION_MULTISET_ALL:

isthmus/src/test/java/io/substrait/isthmus/SubstraitRelNodeConverterTest.java

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package io.substrait.isthmus;
22

3+
import static org.junit.jupiter.api.Assertions.assertThrows;
4+
35
import io.substrait.plan.Plan;
46
import io.substrait.relation.Join.JoinType;
57
import io.substrait.relation.Rel;
@@ -242,6 +244,24 @@ void emit() {
242244
RelNode relNode = substraitToCalcite.convert(root.getInput());
243245
assertRowMatch(relNode.getRowType(), R.I32, N.STRING);
244246
}
247+
248+
// MINUS_MULTISET and INTERSECTION_PRIMARY have no equivalent Calcite relation, so converting
249+
// them to Calcite is unsupported.
250+
@Test
251+
void minusMultisetUnsupported() {
252+
Plan.Root root = sb.root(sb.set(SetOp.MINUS_MULTISET, commonTable, commonTable));
253+
254+
assertThrows(
255+
UnsupportedOperationException.class, () -> substraitToCalcite.convert(root.getInput()));
256+
}
257+
258+
@Test
259+
void intersectionPrimaryUnsupported() {
260+
Plan.Root root = sb.root(sb.set(SetOp.INTERSECTION_PRIMARY, commonTable, commonTable));
261+
262+
assertThrows(
263+
UnsupportedOperationException.class, () -> substraitToCalcite.convert(root.getInput()));
264+
}
245265
}
246266

247267
@Nested

isthmus/src/test/java/io/substrait/isthmus/utils/SetUtils.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,8 @@ private static String asString(Set.SetOp op) {
5959
}
6060

6161
// Generate all SetOp types excluding:
62-
// * MINUS_MULTISET, INTERSECTION_PRIMARY: do not map to Calcite relations
62+
// * MINUS_MULTISET, INTERSECTION_PRIMARY: no equivalent Calcite relation, so they are
63+
// unsupported in Substrait -> Calcite conversion
6364
// * UNKNOWN: invalid
6465
public static Stream<Arguments> setTestConfig() {
6566
return Arrays.stream(Set.SetOp.values())

0 commit comments

Comments
 (0)