Merge pull request #1 from substrait-io/vbarua/sql-to-ddl-suggestions

refactor: suggestions for PR

Author: ZorinAnton

core/src/main/java/io/substrait/util/NoException.java

@@ -16,8 +16,10 @@
 import io.substrait.isthmus.expression.ExpressionRexConverter;
 import io.substrait.isthmus.expression.ScalarFunctionConverter;
 import io.substrait.isthmus.expression.WindowFunctionConverter;
+import io.substrait.relation.AbstractDdlRel;
 import io.substrait.relation.AbstractRelVisitor;
 import io.substrait.relation.AbstractUpdate;
+import io.substrait.relation.AbstractWriteRel;
 import io.substrait.relation.Aggregate;
 import io.substrait.relation.Cross;
 import io.substrait.relation.EmptyScan;
@@ -94,7 +96,6 @@ public class SubstraitRelNodeConverter
   protected final RelBuilder relBuilder;
   protected final RexBuilder rexBuilder;
   private final TypeConverter typeConverter;
-  private final SubstraitRelNodeConverterDdmlValidator substraitRelNodeConverterDdmlValidator;
 
   public SubstraitRelNodeConverter(
       SimpleExtension.ExtensionCollection extensions,
@@ -143,8 +144,6 @@ public SubstraitRelNodeConverter(
     this.aggregateFunctionConverter = aggregateFunctionConverter;
     this.expressionRexConverter = expressionRexConverter;
     this.expressionRexConverter.setRelNodeConverter(this);
-    this.substraitRelNodeConverterDdmlValidator =
-        new SubstraitRelNodeConverterDdmlValidator(relBuilder, this);
   }
 
   public static RelNode convert(
@@ -557,12 +556,21 @@ public RelNode visit(NamedUpdate update, Context context) {
 
   @Override
   public RelNode visit(NamedDdl namedDdl, Context context) {
-    final ValidationResult validationResult =
-        namedDdl.accept(substraitRelNodeConverterDdmlValidator, context);
-    if (!validationResult.isValid()) {
-      throw new IllegalArgumentException(
-          String.join(System.lineSeparator(), validationResult.getMessages()));
+    if (namedDdl.getOperation() != AbstractDdlRel.DdlOp.CREATE
+        || namedDdl.getObject() != AbstractDdlRel.DdlObject.VIEW) {
+      throw new UnsupportedOperationException(
+          String.format(
+              "Can only handle NamedDdl with (%s, %s), given (%s, %s)",
+              AbstractDdlRel.DdlOp.CREATE,
+              AbstractDdlRel.DdlObject.VIEW,
+              namedDdl.getOperation(),
+              namedDdl.getObject()));
     }
+
+    if (namedDdl.getViewDefinition().isEmpty()) {
+      throw new IllegalArgumentException("NamedDdl view definition must be set");
+    }
+
     Rel viewDefinition = namedDdl.getViewDefinition().get();
     RelNode relNode = viewDefinition.accept(this, context);
     RelRoot relRoot = RelRoot.of(relNode, SqlKind.SELECT);
@@ -606,6 +614,17 @@ public RelNode visit(VirtualTableScan virtualTableScan, Context context) {
   }
 
   private RelNode handleCreateTableAs(NamedWrite namedWrite, Context context) {
+    if (namedWrite.getCreateMode() != AbstractWriteRel.CreateMode.REPLACE_IF_EXISTS
+        || namedWrite.getOutputMode() != AbstractWriteRel.OutputMode.NO_OUTPUT) {
+      throw new UnsupportedOperationException(
+          String.format(
+              "Can only handle CTAS NamedWrite with (%s, %s), given (%s, %s)",
+              AbstractWriteRel.CreateMode.REPLACE_IF_EXISTS,
+              AbstractWriteRel.OutputMode.NO_OUTPUT,
+              namedWrite.getCreateMode(),
+              namedWrite.getOutputMode()));
+    }
+
     Rel input = namedWrite.getInput();
     RelNode relNode = input.accept(this, context);
     RelRoot relRoot = RelRoot.of(relNode, SqlKind.SELECT);
@@ -614,12 +633,6 @@ private RelNode handleCreateTableAs(NamedWrite namedWrite, Context context) {
 
   @Override
   public RelNode visit(NamedWrite write, Context context) {
-    final ValidationResult validationResult =
-        write.accept(substraitRelNodeConverterDdmlValidator, context);
-    if (!validationResult.isValid()) {
-      throw new IllegalArgumentException(
-          String.join(System.lineSeparator(), validationResult.getMessages()));
-    }
     RelNode input = write.getInput().accept(this, context);
     assert relBuilder.getRelOptSchema() != null;
     final RelOptTable targetTable =
@@ -636,8 +649,10 @@ public RelNode visit(NamedWrite write, Context context) {
       case CTAS:
         return handleCreateTableAs(write, context);
       default:
-        // checked by validation
-        throw new IllegalArgumentException("Couldn't determine operation");
+        throw new UnsupportedOperationException(
+            String.format(
+                "NamedWrite with WriteOp %s cannot be converted to a Calcite RelNode. Consider using a more specific Rel (e.g NamedUpdate)",
+                write.getOperation()));
     }
 
     // checked by validation

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

@@ -465,7 +465,7 @@ public Rel handleCreateTable(CreateTable createTable) {
         .operation(AbstractWriteRel.WriteOp.CTAS)
         .createMode(AbstractWriteRel.CreateMode.REPLACE_IF_EXISTS)
         .outputMode(AbstractWriteRel.OutputMode.NO_OUTPUT)
-        .names(createTable.getNames())
+        .names(createTable.getTableName())
         .build();
   }
 
@@ -481,7 +481,7 @@ public Rel handleCreateView(CreateView createView) {
         .tableDefaults(defaults)
         .operation(AbstractDdlRel.DdlOp.CREATE)
         .object(AbstractDdlRel.DdlObject.VIEW)
-        .names(createView.getNames())
+        .names(createView.getViewName())
         .build();
   }
 

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

@@ -2,40 +2,43 @@
 
 import java.util.List;
 import org.apache.calcite.rel.AbstractRelNode;
+import org.apache.calcite.rel.RelNode;
 import org.apache.calcite.rel.RelRoot;
+import org.apache.calcite.rel.RelWriter;
 import org.apache.calcite.rel.type.RelDataType;
 
 public class CreateTable extends AbstractRelNode {
 
-  private List<String> names;
-  private RelRoot input;
+  private final List<String> tableName;
+  private final RelRoot input;
 
-  public RelRoot getInput() {
-    return input;
-  }
-
-  public void setInput(RelRoot input) {
-    this.input = input;
-  }
-
-  public CreateTable(List<String> names, RelRoot input) {
+  public CreateTable(List<String> tableName, RelRoot input) {
     super(input.rel.getCluster(), input.rel.getTraitSet());
 
-    this.names = names;
+    this.tableName = tableName;
     this.input = input;
   }
 
   @Override
   protected RelDataType deriveRowType() {
-    // return new DdlRelDataType();
     return input.validatedRowType;
   }
 
-  public List<String> getNames() {
-    return names;
+  @Override
+  public RelWriter explainTerms(RelWriter pw) {
+    return super.explainTerms(pw).input("input", getInput().rel).item("tableName", getTableName());
   }
 
-  public void setNames(List<String> names) {
-    this.names = names;
+  @Override
+  public List<RelNode> getInputs() {
+    return List.of(input.rel);
+  }
+
+  public List<String> getTableName() {
+    return tableName;
+  }
+
+  public RelRoot getInput() {
+    return input;
   }
 }

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

@@ -2,38 +2,41 @@
 
 import java.util.List;
 import org.apache.calcite.rel.AbstractRelNode;
+import org.apache.calcite.rel.RelNode;
 import org.apache.calcite.rel.RelRoot;
+import org.apache.calcite.rel.RelWriter;
 import org.apache.calcite.rel.type.RelDataType;
 
 public class CreateView extends AbstractRelNode {
-  private List<String> names;
-  private RelRoot input;
+  private final List<String> viewName;
+  private final RelRoot input;
 
-  public CreateView(List<String> names, RelRoot input) {
+  public CreateView(List<String> viewName, RelRoot input) {
     super(input.rel.getCluster(), input.rel.getTraitSet());
-    this.names = names;
+    this.viewName = viewName;
     this.input = input;
   }
 
   @Override
   protected RelDataType deriveRowType() {
-    // return new DdlRelDataType();
     return input.validatedRowType;
   }
 
-  public List<String> getNames() {
-    return names;
+  @Override
+  public RelWriter explainTerms(RelWriter pw) {
+    return super.explainTerms(pw).input("input", getInput().rel).item("viewName", getViewName());
   }
 
-  public void setNames(List<String> names) {
-    this.names = names;
+  @Override
+  public List<RelNode> getInputs() {
+    return List.of(input.rel);
   }
 
-  public RelRoot getInput() {
-    return input;
+  public List<String> getViewName() {
+    return viewName;
   }
 
-  public void setInput(RelRoot input) {
-    this.input = input;
+  public RelRoot getInput() {
+    return input;
   }
 }

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

@@ -18,12 +18,12 @@ public DdlRoundtripTest() throws SqlParseException {
   @Test
   void testCreateTable() throws Exception {
     String sql = "create table dst1 as select * from src1";
-    assertFullRoundTripWorkaroundOptimizer(sql, catalogReader);
+    assertFullRoundTripWithIdentityProjectionWorkaround(sql, catalogReader);
   }
 
   @Test
   void testCreateView() throws Exception {
     String sql = "create view dst1 as select * from src1";
-    assertFullRoundTripWorkaroundOptimizer(sql, catalogReader);
+    assertFullRoundTripWithIdentityProjectionWorkaround(sql, catalogReader);
   }
 }

isthmus/src/main/java/io/substrait/isthmus/calcite/rel/CreateTable.java

@@ -16,20 +16,21 @@ public DmlRoundtripTest() throws SqlParseException {}
 
   @Test
   void testDelete() throws SqlParseException {
-    assertFullRoundTripWorkaroundOptimizer("delete from src1 where intcol=10", catalogReader);
+    assertFullRoundTripWithIdentityProjectionWorkaround(
+        "delete from src1 where intcol=10", catalogReader);
   }
 
   @Test
   void testUpdate() throws SqlParseException {
-    assertFullRoundTripWorkaroundOptimizer(
+    assertFullRoundTripWithIdentityProjectionWorkaround(
         "update src1 set intcol=10 where charcol='a'", catalogReader);
   }
 
   @Test
   void testInsert() throws SqlParseException {
-    assertFullRoundTripWorkaroundOptimizer(
+    assertFullRoundTripWithIdentityProjectionWorkaround(
         "insert into src1 (intcol, charcol) values (1,'a'); ", catalogReader);
-    assertFullRoundTripWorkaroundOptimizer(
+    assertFullRoundTripWithIdentityProjectionWorkaround(
         "insert into src1 (intcol, charcol) select intcol,charcol from src2;", catalogReader);
   }
 }