feat(isthmus): always emit Cross relation when possible (#367)

BREAKING CHANGE: removed CrossJoinPolicy from FeatureBoard

Author: vbarua

isthmus-cli/src/main/java/io/substrait/isthmus/cli/IsthmusEntryPoint.java

@@ -13,7 +13,6 @@
 import io.substrait.isthmus.ImmutableFeatureBoard;
 import io.substrait.isthmus.SqlExpressionToSubstrait;
 import io.substrait.isthmus.SqlToSubstrait;
-import io.substrait.isthmus.SubstraitRelVisitor.CrossJoinPolicy;
 import io.substrait.proto.ExtendedExpression;
 import io.substrait.proto.Plan;
 import java.io.IOException;
@@ -66,11 +65,6 @@ enum OutputFormat {
       description = "One of built-in Calcite SQL compatibility modes: ${COMPLETION-CANDIDATES}")
   private SqlConformanceEnum sqlConformanceMode = SqlConformanceEnum.DEFAULT;
 
-  @Option(
-      names = {"--crossjoinpolicy"},
-      description = "One of built-in Calcite SQL compatibility modes: ${COMPLETION-CANDIDATES}")
-  private CrossJoinPolicy crossJoinPolicy = CrossJoinPolicy.KEEP_AS_CROSS_JOIN;
-
   @Option(
       names = {"--unquotedcasing"},
       description = "Calcite's casing policy for unquoted identifiers: ${COMPLETION-CANDIDATES}")
@@ -127,7 +121,6 @@ FeatureBoard buildFeatureBoard() {
     return ImmutableFeatureBoard.builder()
         .allowsSqlBatch(allowMultiStatement)
         .sqlConformanceMode(sqlConformanceMode)
-        .crossJoinPolicy(crossJoinPolicy)
         .unquotedCasing(unquotedCasing)
         .build();
   }

isthmus-cli/src/test/java/io/substrait/isthmus/cli/IsthmusEntryPointTest.java

@@ -6,7 +6,6 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import io.substrait.isthmus.FeatureBoard;
-import io.substrait.isthmus.SubstraitRelVisitor.CrossJoinPolicy;
 import org.apache.calcite.sql.validate.SqlConformance;
 import org.apache.calcite.sql.validate.SqlConformanceEnum;
 import org.junit.jupiter.api.Test;
@@ -23,24 +22,18 @@ void defaultFeatureBoard() {
     FeatureBoard features = isthmusEntryPoint.buildFeatureBoard();
     assertFalse(features.allowsSqlBatch());
     assertEquals(SqlConformanceEnum.DEFAULT, features.sqlConformanceMode());
-    assertEquals(CrossJoinPolicy.KEEP_AS_CROSS_JOIN, features.crossJoinPolicy());
   }
 
   /** Test that the command line options are correctly parsed into the {@link FeatureBoard}. */
   @Test
   void customFeatureBoard() {
     IsthmusEntryPoint isthmusEntryPoint = new IsthmusEntryPoint();
     new CommandLine(isthmusEntryPoint)
-        .parseArgs(
-            "--multistatement",
-            "--sqlconformancemode=SQL_SERVER_2008",
-            "--crossjoinpolicy=CONVERT_TO_INNER_JOIN",
-            "SELECT * FROM foo");
+        .parseArgs("--multistatement", "--sqlconformancemode=SQL_SERVER_2008", "SELECT * FROM foo");
     FeatureBoard features = isthmusEntryPoint.buildFeatureBoard();
     assertTrue(features.allowsSqlBatch());
     assertEquals(
         (SqlConformance) SqlConformanceEnum.SQL_SERVER_2008, features.sqlConformanceMode());
-    assertEquals(CrossJoinPolicy.CONVERT_TO_INNER_JOIN, features.crossJoinPolicy());
   }
 
   /**

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

@@ -1,6 +1,5 @@
 package io.substrait.isthmus;
 
-import io.substrait.isthmus.SubstraitRelVisitor.CrossJoinPolicy;
 import org.apache.calcite.avatica.util.Casing;
 import org.apache.calcite.sql.validate.SqlConformance;
 import org.apache.calcite.sql.validate.SqlConformanceEnum;
@@ -33,14 +32,6 @@ public SqlConformance sqlConformanceMode() {
     return SqlConformanceEnum.DEFAULT;
   }
 
-  /**
-   * @return the selected cross join policy
-   */
-  @Value.Default
-  public CrossJoinPolicy crossJoinPolicy() {
-    return CrossJoinPolicy.KEEP_AS_CROSS_JOIN;
-  }
-
   /**
    * @return Calcite's identifier casing policy for unquoted identifiers.
    */

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

@@ -1,7 +1,5 @@
 package io.substrait.isthmus;
 
-import static io.substrait.isthmus.SubstraitRelVisitor.CrossJoinPolicy.KEEP_AS_CROSS_JOIN;
-
 import io.substrait.expression.Expression;
 import io.substrait.expression.ExpressionCreator;
 import io.substrait.expression.FieldReference;
@@ -183,9 +181,8 @@ public Rel visit(org.apache.calcite.rel.core.Join join) {
               "Unsupported join type: " + join.getJoinType());
         };
 
-    if (joinType == Join.JoinType.INNER
-        && TRUE.equals(condition)
-        && featureBoard.crossJoinPolicy().equals(KEEP_AS_CROSS_JOIN)) {
+    // An INNER JOIN with a join condition of TRUE can be encoded as a Substrait Cross relation
+    if (joinType == Join.JoinType.INNER && TRUE.equals(condition)) {
       return Cross.builder().left(left).right(right).build();
     }
     return Join.builder().condition(condition).joinType(joinType).left(left).right(right).build();
@@ -398,29 +395,4 @@ private static Rel convert(
     visitor.popFieldAccessDepthMap(rel);
     return visitor.apply(rel);
   }
-
-  public enum CrossJoinPolicy {
-    KEEP_AS_CROSS_JOIN,
-    CONVERT_TO_INNER_JOIN
-  };
-
-  public static class Options {
-    private final CrossJoinPolicy crossJoinPolicy;
-
-    public Options() {
-      this(CrossJoinPolicy.CONVERT_TO_INNER_JOIN);
-    }
-
-    public Options(CrossJoinPolicy crossJoinPolicy) {
-      this.crossJoinPolicy = crossJoinPolicy;
-    }
-
-    /**
-     * Returns the {@code crossJoinAsInnerJoin} option. Controls whether to cross joins are
-     * represented as inner joins with a true condition.
-     */
-    public CrossJoinPolicy getCrossJoinPolicy() {
-      return crossJoinPolicy;
-    }
-  }
 }

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

@@ -1,6 +1,5 @@
 package io.substrait.isthmus;
 
-import static io.substrait.isthmus.SubstraitRelVisitor.CrossJoinPolicy.KEEP_AS_CROSS_JOIN;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 import io.substrait.isthmus.utils.SetUtils;
@@ -73,36 +72,41 @@ public void filter() throws IOException, SqlParseException {
   @Test
   public void crossJoin() throws IOException, SqlParseException {
     int[] counter = new int[1];
-    var visitor =
+    var crossJoinCountingVisitor =
         new RelCopyOnWriteVisitor<RuntimeException>() {
           public Optional<Rel> visit(Cross cross) throws RuntimeException {
             counter[0]++;
             return super.visit(cross);
           }
         };
-    FeatureBoard featureBoard =
-        ImmutableFeatureBoard.builder().crossJoinPolicy(KEEP_AS_CROSS_JOIN).build();
+    var featureBoard = ImmutableFeatureBoard.builder().build();
+
     Plan plan1 =
         assertProtoPlanRoundrip(
-            "select\n"
-                + "  c.c_custKey,\n"
-                + "  o.o_custkey\n"
-                + "from\n"
-                + "  \"customer\" c cross join\n"
-                + "  \"orders\" o",
+            """
+            select
+              c.c_custKey,
+              o.o_custkey
+            from
+              "customer" c cross join
+              "orders" o
+            """,
             new SqlToSubstrait(featureBoard));
-    plan1.getRoots().forEach(t -> t.getInput().accept(visitor));
+    plan1.getRoots().forEach(t -> t.getInput().accept(crossJoinCountingVisitor));
     assertEquals(1, counter[0]);
+
     Plan plan2 =
         assertProtoPlanRoundrip(
-            "select\n"
-                + "  c.c_custKey,\n"
-                + "  o.o_custkey\n"
-                + "from\n"
-                + "  \"customer\" c,\n"
-                + "  \"orders\" o",
+            """
+            select
+              c.c_custKey,
+              o.o_custkey
+            from
+              "customer" c,
+              "orders" o
+            """,
             new SqlToSubstrait(featureBoard));
-    plan2.getRoots().forEach(t -> t.getInput().accept(visitor));
+    plan2.getRoots().forEach(t -> t.getInput().accept(crossJoinCountingVisitor));
     assertEquals(2, counter[0]);
   }