diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java index 0aef61857..b39fce72a 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java @@ -285,10 +285,6 @@ public RelNode visit(Set set, Context context) throws RuntimeException { input -> { relBuilder.push(input.accept(this, context)); }); - // TODO: MINUS_MULTISET and INTERSECTION_PRIMARY mappings are set to be removed as they do not - // correspond to the Calcite relations they are associated with. They are retained for now - // to enable users to migrate off of them. - // See: https://github.com/substrait-io/substrait-java/issues/303 RelBuilder builder = getRelBuilder(set); RelNode node = builder.build(); return applyRemap(node, set.getRemap()); @@ -301,9 +297,7 @@ private RelBuilder getRelBuilder(Set set) { case MINUS_PRIMARY: return relBuilder.minus(false, numInputs); case MINUS_PRIMARY_ALL: - case MINUS_MULTISET: return relBuilder.minus(true, numInputs); - case INTERSECTION_PRIMARY: case INTERSECTION_MULTISET: return relBuilder.intersect(false, numInputs); case INTERSECTION_MULTISET_ALL: diff --git a/isthmus/src/test/java/io/substrait/isthmus/SubstraitRelNodeConverterTest.java b/isthmus/src/test/java/io/substrait/isthmus/SubstraitRelNodeConverterTest.java index 7ac6f9f66..4a5ed1f9e 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/SubstraitRelNodeConverterTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/SubstraitRelNodeConverterTest.java @@ -1,5 +1,7 @@ package io.substrait.isthmus; +import static org.junit.jupiter.api.Assertions.assertThrows; + import io.substrait.plan.Plan; import io.substrait.relation.Join.JoinType; import io.substrait.relation.Rel; @@ -242,6 +244,24 @@ void emit() { RelNode relNode = substraitToCalcite.convert(root.getInput()); assertRowMatch(relNode.getRowType(), R.I32, N.STRING); } + + // MINUS_MULTISET and INTERSECTION_PRIMARY have no equivalent Calcite relation, so converting + // them to Calcite is unsupported. + @Test + void minusMultisetUnsupported() { + Plan.Root root = sb.root(sb.set(SetOp.MINUS_MULTISET, commonTable, commonTable)); + + assertThrows( + UnsupportedOperationException.class, () -> substraitToCalcite.convert(root.getInput())); + } + + @Test + void intersectionPrimaryUnsupported() { + Plan.Root root = sb.root(sb.set(SetOp.INTERSECTION_PRIMARY, commonTable, commonTable)); + + assertThrows( + UnsupportedOperationException.class, () -> substraitToCalcite.convert(root.getInput())); + } } @Nested diff --git a/isthmus/src/test/java/io/substrait/isthmus/utils/SetUtils.java b/isthmus/src/test/java/io/substrait/isthmus/utils/SetUtils.java index 3e57905e2..5d8b5a089 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/utils/SetUtils.java +++ b/isthmus/src/test/java/io/substrait/isthmus/utils/SetUtils.java @@ -59,7 +59,8 @@ private static String asString(Set.SetOp op) { } // Generate all SetOp types excluding: - // * MINUS_MULTISET, INTERSECTION_PRIMARY: do not map to Calcite relations + // * MINUS_MULTISET, INTERSECTION_PRIMARY: no equivalent Calcite relation, so they are + // unsupported in Substrait -> Calcite conversion // * UNKNOWN: invalid public static Stream setTestConfig() { return Arrays.stream(Set.SetOp.values())