From 96b01970adf9090d16fd92426a5129755a47a0e4 Mon Sep 17 00:00:00 2001 From: Kadin Rabo Date: Wed, 2 Oct 2024 14:45:26 -0400 Subject: [PATCH 1/4] chore: update to substrait v0.57.0 --- core/src/main/java/io/substrait/relation/Set.java | 2 ++ .../io/substrait/isthmus/SubstraitRelNodeConverter.java | 7 ++++--- .../java/io/substrait/isthmus/SubstraitRelVisitor.java | 5 +++-- .../src/test/java/io/substrait/isthmus/utils/SetUtils.java | 6 +++--- substrait | 2 +- 5 files changed, 13 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/io/substrait/relation/Set.java b/core/src/main/java/io/substrait/relation/Set.java index 6160c2460..4efef01e2 100644 --- a/core/src/main/java/io/substrait/relation/Set.java +++ b/core/src/main/java/io/substrait/relation/Set.java @@ -11,9 +11,11 @@ public abstract class Set extends AbstractRel implements HasExtension { public static enum SetOp { UNKNOWN(SetRel.SetOp.SET_OP_UNSPECIFIED), MINUS_PRIMARY(SetRel.SetOp.SET_OP_MINUS_PRIMARY), + MINUS_PRIMARY_ALL(SetRel.SetOp.SET_OP_MINUS_PRIMARY_ALL), MINUS_MULTISET(SetRel.SetOp.SET_OP_MINUS_MULTISET), INTERSECTION_PRIMARY(SetRel.SetOp.SET_OP_INTERSECTION_PRIMARY), INTERSECTION_MULTISET(SetRel.SetOp.SET_OP_INTERSECTION_MULTISET), + INTERSECTION_MULTISET_ALL(SetRel.SetOp.SET_OP_INTERSECTION_MULTISET_ALL), UNION_DISTINCT(SetRel.SetOp.SET_OP_UNION_DISTINCT), UNION_ALL(SetRel.SetOp.SET_OP_UNION_ALL); diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java index 8691466c6..e9ec9917e 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java @@ -214,9 +214,10 @@ public RelNode visit(Set set) throws RuntimeException { var builder = switch (set.getSetOp()) { case MINUS_PRIMARY -> relBuilder.minus(false, numInputs); - case MINUS_MULTISET -> relBuilder.minus(true, numInputs); - case INTERSECTION_PRIMARY -> relBuilder.intersect(false, numInputs); - case INTERSECTION_MULTISET -> relBuilder.intersect(true, numInputs); + case MINUS_PRIMARY_ALL, MINUS_MULTISET -> relBuilder.minus(true, numInputs); + case INTERSECTION_PRIMARY, INTERSECTION_MULTISET -> relBuilder.intersect( + false, numInputs); + case INTERSECTION_MULTISET_ALL -> relBuilder.intersect(true, numInputs); case UNION_DISTINCT -> relBuilder.union(false, numInputs); case UNION_ALL -> relBuilder.union(true, numInputs); case UNKNOWN -> throw new UnsupportedOperationException( diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java index cd10a7ef3..a367e0217 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java @@ -217,14 +217,15 @@ public Rel visit(org.apache.calcite.rel.core.Union union) { @Override public Rel visit(org.apache.calcite.rel.core.Intersect intersect) { var inputs = apply(intersect.getInputs()); - var setOp = intersect.all ? Set.SetOp.INTERSECTION_MULTISET : Set.SetOp.INTERSECTION_PRIMARY; + var setOp = + intersect.all ? Set.SetOp.INTERSECTION_MULTISET_ALL : Set.SetOp.INTERSECTION_MULTISET; return Set.builder().inputs(inputs).setOp(setOp).build(); } @Override public Rel visit(org.apache.calcite.rel.core.Minus minus) { var inputs = apply(minus.getInputs()); - var setOp = minus.all ? Set.SetOp.MINUS_MULTISET : Set.SetOp.MINUS_PRIMARY; + var setOp = minus.all ? Set.SetOp.MINUS_PRIMARY_ALL : Set.SetOp.MINUS_PRIMARY; return Set.builder().inputs(inputs).setOp(setOp).build(); } 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 b7cc0d141..c154d21e7 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/utils/SetUtils.java +++ b/isthmus/src/test/java/io/substrait/isthmus/utils/SetUtils.java @@ -21,9 +21,9 @@ public static String getSetQuery(Set.SetOp op, boolean multi) { String opString = switch (op) { case MINUS_PRIMARY -> "EXCEPT"; - case MINUS_MULTISET -> "EXCEPT ALL"; - case INTERSECTION_PRIMARY -> "INTERSECT"; - case INTERSECTION_MULTISET -> "INTERSECT ALL"; + case MINUS_PRIMARY_ALL, MINUS_MULTISET -> "EXCEPT ALL"; + case INTERSECTION_PRIMARY, INTERSECTION_MULTISET -> "INTERSECT"; + case INTERSECTION_MULTISET_ALL -> "INTERSECT ALL"; case UNION_DISTINCT -> "UNION"; case UNION_ALL -> "UNION ALL"; case UNKNOWN -> throw new UnsupportedOperationException( diff --git a/substrait b/substrait index bc4d6fb9b..297715c7b 160000 --- a/substrait +++ b/substrait @@ -1 +1 @@ -Subproject commit bc4d6fb9bc0435c3db24172566c343e119fc50a9 +Subproject commit 297715c7b7b6cb367973cc64c1a2a296df12a812 From 45d3e8d1028a65d4d8ac67492850ad7e1ec231b6 Mon Sep 17 00:00:00 2001 From: Kadin Rabo Date: Fri, 4 Oct 2024 09:28:33 -0400 Subject: [PATCH 2/4] document MINUS_MULTISET and INTERSECTION_PRIMARY --- core/src/main/java/io/substrait/relation/Set.java | 1 + .../isthmus/SubstraitRelNodeConverter.java | 2 ++ .../java/io/substrait/isthmus/utils/SetUtils.java | 13 +++++++++---- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/io/substrait/relation/Set.java b/core/src/main/java/io/substrait/relation/Set.java index 4efef01e2..9b1f9992b 100644 --- a/core/src/main/java/io/substrait/relation/Set.java +++ b/core/src/main/java/io/substrait/relation/Set.java @@ -8,6 +8,7 @@ public abstract class Set extends AbstractRel implements HasExtension { public abstract Set.SetOp getSetOp(); + // MINUS_MULTISET and INTERSECTION_PRIMARY are set to be removed (substrait-io/substrait/pull/708) public static enum SetOp { UNKNOWN(SetRel.SetOp.SET_OP_UNSPECIFIED), MINUS_PRIMARY(SetRel.SetOp.SET_OP_MINUS_PRIMARY), diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java index e9ec9917e..da2b83119 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java @@ -211,6 +211,8 @@ public RelNode visit(Set set) throws RuntimeException { input -> { relBuilder.push(input.accept(this)); }); + // MINUS_MULTISET and INTERSECTION_PRIMARY are set to be removed + // (substrait-io/substrait/pull/708) var builder = switch (set.getSetOp()) { case MINUS_PRIMARY -> relBuilder.minus(false, numInputs); 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 c154d21e7..15a0603dc 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/utils/SetUtils.java +++ b/isthmus/src/test/java/io/substrait/isthmus/utils/SetUtils.java @@ -21,12 +21,12 @@ public static String getSetQuery(Set.SetOp op, boolean multi) { String opString = switch (op) { case MINUS_PRIMARY -> "EXCEPT"; - case MINUS_PRIMARY_ALL, MINUS_MULTISET -> "EXCEPT ALL"; - case INTERSECTION_PRIMARY, INTERSECTION_MULTISET -> "INTERSECT"; + case MINUS_PRIMARY_ALL -> "EXCEPT ALL"; + case INTERSECTION_MULTISET -> "INTERSECT"; case INTERSECTION_MULTISET_ALL -> "INTERSECT ALL"; case UNION_DISTINCT -> "UNION"; case UNION_ALL -> "UNION ALL"; - case UNKNOWN -> throw new UnsupportedOperationException( + default -> throw new UnsupportedOperationException( "Unknown set operation is not supported"); }; @@ -50,9 +50,14 @@ public static String getSetQuery(Set.SetOp op, boolean multi) { } // Generate all combinations excluding the UNKNOWN operator + // MINUS_MULTISET and INTERSECTION_PRIMARY are set to be removed (substrait-io/substrait/pull/708) public static Stream setTestConfig() { return Arrays.stream(Set.SetOp.values()) - .filter(op -> op != Set.SetOp.UNKNOWN) + .filter( + op -> + op != Set.SetOp.UNKNOWN + && op != Set.SetOp.MINUS_MULTISET + && op != Set.SetOp.INTERSECTION_PRIMARY) .flatMap(op -> Stream.of(arguments(op, false), arguments(op, true))); } } From d70f7d996c9230855e4791205b689ba92a4de9cd Mon Sep 17 00:00:00 2001 From: Kadin Rabo Date: Fri, 4 Oct 2024 11:12:49 -0400 Subject: [PATCH 3/4] chore: comment --- core/src/main/java/io/substrait/relation/Set.java | 2 +- .../java/io/substrait/isthmus/SubstraitRelNodeConverter.java | 3 +-- isthmus/src/test/java/io/substrait/isthmus/utils/SetUtils.java | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/io/substrait/relation/Set.java b/core/src/main/java/io/substrait/relation/Set.java index 9b1f9992b..2ddb24854 100644 --- a/core/src/main/java/io/substrait/relation/Set.java +++ b/core/src/main/java/io/substrait/relation/Set.java @@ -8,7 +8,7 @@ public abstract class Set extends AbstractRel implements HasExtension { public abstract Set.SetOp getSetOp(); - // MINUS_MULTISET and INTERSECTION_PRIMARY are set to be removed (substrait-io/substrait/pull/708) + // MINUS_MULTISET and INTERSECTION_PRIMARY mappings are set to be removed due to no direct SQL mapping (substrait-io/substrait/pull/708) public static enum SetOp { UNKNOWN(SetRel.SetOp.SET_OP_UNSPECIFIED), MINUS_PRIMARY(SetRel.SetOp.SET_OP_MINUS_PRIMARY), diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java index da2b83119..8b75ebcd2 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java @@ -211,8 +211,7 @@ public RelNode visit(Set set) throws RuntimeException { input -> { relBuilder.push(input.accept(this)); }); - // MINUS_MULTISET and INTERSECTION_PRIMARY are set to be removed - // (substrait-io/substrait/pull/708) + // MINUS_MULTISET and INTERSECTION_PRIMARY mappings are set to be removed due to no direct SQL mapping (substrait-io/substrait/pull/708) var builder = switch (set.getSetOp()) { case MINUS_PRIMARY -> relBuilder.minus(false, numInputs); 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 15a0603dc..7c64a7f74 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/utils/SetUtils.java +++ b/isthmus/src/test/java/io/substrait/isthmus/utils/SetUtils.java @@ -50,7 +50,7 @@ public static String getSetQuery(Set.SetOp op, boolean multi) { } // Generate all combinations excluding the UNKNOWN operator - // MINUS_MULTISET and INTERSECTION_PRIMARY are set to be removed (substrait-io/substrait/pull/708) + // Note: MINUS_MULTISET and INTERSECTION_PRIMARY mappings to Calcite are set to be removed due to no direct SQL mapping (substrait-io/substrait/pull/708) public static Stream setTestConfig() { return Arrays.stream(Set.SetOp.values()) .filter( From 413feded666bd9b84f93645200856880ca97ed3d Mon Sep 17 00:00:00 2001 From: Victor Barua Date: Fri, 4 Oct 2024 08:28:40 -0700 Subject: [PATCH 4/4] docs: minor doc updates --- core/src/main/java/io/substrait/relation/Set.java | 1 - .../java/io/substrait/isthmus/SubstraitRelNodeConverter.java | 5 ++++- .../src/test/java/io/substrait/isthmus/utils/SetUtils.java | 5 +++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/io/substrait/relation/Set.java b/core/src/main/java/io/substrait/relation/Set.java index 2ddb24854..4efef01e2 100644 --- a/core/src/main/java/io/substrait/relation/Set.java +++ b/core/src/main/java/io/substrait/relation/Set.java @@ -8,7 +8,6 @@ public abstract class Set extends AbstractRel implements HasExtension { public abstract Set.SetOp getSetOp(); - // MINUS_MULTISET and INTERSECTION_PRIMARY mappings are set to be removed due to no direct SQL mapping (substrait-io/substrait/pull/708) public static enum SetOp { UNKNOWN(SetRel.SetOp.SET_OP_UNSPECIFIED), MINUS_PRIMARY(SetRel.SetOp.SET_OP_MINUS_PRIMARY), diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java index 8b75ebcd2..3cdbfd3d1 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java @@ -211,7 +211,10 @@ public RelNode visit(Set set) throws RuntimeException { input -> { relBuilder.push(input.accept(this)); }); - // MINUS_MULTISET and INTERSECTION_PRIMARY mappings are set to be removed due to no direct SQL mapping (substrait-io/substrait/pull/708) + // 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 var builder = switch (set.getSetOp()) { case MINUS_PRIMARY -> relBuilder.minus(false, numInputs); 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 7c64a7f74..deac90e87 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/utils/SetUtils.java +++ b/isthmus/src/test/java/io/substrait/isthmus/utils/SetUtils.java @@ -49,8 +49,9 @@ public static String getSetQuery(Set.SetOp op, boolean multi) { } } - // Generate all combinations excluding the UNKNOWN operator - // Note: MINUS_MULTISET and INTERSECTION_PRIMARY mappings to Calcite are set to be removed due to no direct SQL mapping (substrait-io/substrait/pull/708) + // Generate all SetOp types excluding: + // * MINUS_MULTISET, INTERSECTION_PRIMARY: do not map to Calcite relations + // * UNKNOWN: invalid public static Stream setTestConfig() { return Arrays.stream(Set.SetOp.values()) .filter(