[CALCITE-7301] Add common test for SqlNode unparse after deep copy and fix operators missing createCall

babel/src/main/java/org/apache/calcite/sql/babel/SqlBabelCreateTable.java

@@ -17,17 +17,39 @@
 package org.apache.calcite.sql.babel;
 
 import org.apache.calcite.sql.SqlIdentifier;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.SqlLiteral;
 import org.apache.calcite.sql.SqlNode;
 import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlOperator;
 import org.apache.calcite.sql.SqlWriter;
 import org.apache.calcite.sql.ddl.SqlCreateTable;
+import org.apache.calcite.sql.fun.SqlBasicOperator;
 import org.apache.calcite.sql.parser.SqlParserPos;
+import org.apache.calcite.util.ImmutableNullableList;
+
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+import java.util.List;
+
+import static java.util.Objects.requireNonNull;
 
 /**
  * Parse tree for {@code CREATE TABLE} statement, with extensions for particular
  * SQL dialects supported by Babel.
  */
 public class SqlBabelCreateTable extends SqlCreateTable {
+  private static final SqlOperator OPERATOR =
+      SqlBasicOperator.create("CREATE TABLE", SqlKind.CREATE_TABLE).withCallFactory(
+          (operator, functionQualifier, pos, operands) ->
+              new SqlBabelCreateTable(pos,
+                  requireNonNull((SqlLiteral) operands[0]).booleanValue(),
+                  requireNonNull((SqlLiteral) operands[1]).symbolValue(TableCollectionType.class),
+                  requireNonNull((SqlLiteral) operands[2]).booleanValue(),
+                  requireNonNull((SqlLiteral) operands[3]).booleanValue(),
+                  (SqlIdentifier) requireNonNull(operands[4]),
+                  (SqlNodeList) operands[5], operands[6]));
+
   private final TableCollectionType tableCollectionType;
   // CHECKSTYLE: IGNORE 2; can't use 'volatile' because it is a Java keyword
   // but checkstyle does not like trailing '_'.
@@ -36,13 +58,21 @@ public class SqlBabelCreateTable extends SqlCreateTable {
   /** Creates a SqlBabelCreateTable. */
   public SqlBabelCreateTable(SqlParserPos pos, boolean replace,
       TableCollectionType tableCollectionType, boolean volatile_,
-      boolean ifNotExists, SqlIdentifier name, SqlNodeList columnList,
-      SqlNode query) {
-    super(pos, replace, ifNotExists, name, columnList, query);
+      boolean ifNotExists, SqlIdentifier name, @Nullable SqlNodeList columnList,
+      @Nullable SqlNode query) {
+    super(OPERATOR, pos, replace, ifNotExists, name, columnList, query);
     this.tableCollectionType = tableCollectionType;
     this.volatile_ = volatile_;
   }
 
+  @SuppressWarnings("nullness")
+  @Override public List<SqlNode> getOperandList() {
+    return ImmutableNullableList.of(SqlLiteral.createBoolean(getReplace(), pos),
+        SqlLiteral.createSymbol(tableCollectionType, pos),
+        SqlLiteral.createBoolean(volatile_, pos),
+        SqlLiteral.createBoolean(ifNotExists, pos), name, columnList, query);
+  }
+
   @Override public void unparse(SqlWriter writer, int leftPrec, int rightPrec) {
     writer.keyword("CREATE");
     switch (tableCollectionType) {

babel/src/test/java/org/apache/calcite/test/BabelParserTest.java

@@ -40,6 +40,7 @@
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.hasToString;
+import static org.junit.jupiter.api.Assumptions.assumeFalse;
 
 /**
  * Tests the "Babel" SQL parser, that understands all dialects of SQL.
@@ -415,7 +416,12 @@ private void checkParseInfixCast(String sqlType) {
   }
 
   @Test void testPostgresSqlSetOption() {
-    SqlParserFixture f = fixture().withDialect(PostgresqlSqlDialect.DEFAULT);
+    // UnparsingTesterImpl has a check where it unparses a SqlNode into a SQL string
+    // using the calcite dialect, and then parses it back into a SqlNode.
+    // But the SQL string produced by the calcite dialect for `SET` cannot always be parsed back.
+    assumeFalse(fixture().tester.isUnparserTest());
+    SqlParserFixture f = fixture()
+        .withDialect(PostgresqlSqlDialect.DEFAULT);
     f.sql("SET SESSION autovacuum = true")
         .ok("SET \"autovacuum\" = TRUE");
     f.sql("SET SESSION autovacuum = DEFAULT")

babel/src/test/java/org/apache/calcite/test/BabelUnParserTest.java

@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.test;
+
+import org.apache.calcite.sql.parser.SqlParserFixture;
+
+/**
+ * Extension to {@link BabelParserTest} that ensures that every expression can
+ * un-parse successfully.
+ */
+public class BabelUnParserTest extends BabelParserTest {
+  @Override public SqlParserFixture fixture() {
+    return super.fixture()
+        .withTester(new UnparsingTesterImpl());
+  }
+}

core/src/main/java/org/apache/calcite/sql/SqlMerge.java

@@ -28,13 +28,24 @@
 
 import java.util.List;
 
+import static java.util.Objects.requireNonNull;
+
 /**
  * A <code>SqlMerge</code> is a node of a parse tree which represents a MERGE
  * statement.
  */
 public class SqlMerge extends SqlCall {
   public static final SqlSpecialOperator OPERATOR =
-      new SqlSpecialOperator("MERGE", SqlKind.MERGE);
+      new SqlSpecialOperator("MERGE", SqlKind.MERGE) {
+        @Override public SqlCall createCall(final @Nullable SqlLiteral functionQualifier,
+            final SqlParserPos pos,
+            final @Nullable SqlNode... operands) {
+          return new SqlMerge(pos, requireNonNull(operands[0]), requireNonNull(operands[1]),
+              requireNonNull(operands[2]),
+              (SqlUpdate) operands[3], (SqlInsert) operands[4],
+              (SqlSelect) operands[5], (SqlIdentifier) operands[6]);
+        }
+      };
 
   SqlNode targetTable;
   SqlNode condition;

core/src/main/java/org/apache/calcite/sql/SqlSetOption.java

@@ -143,7 +143,7 @@ public SqlSetOption(SqlParserPos pos, @Nullable String scope, SqlIdentifier name
     } else {
       operandList.add(new SqlIdentifier(scope, SqlParserPos.ZERO));
     }
-    operandList.add(name);
+    operandList.add(nameAsSqlNode);
     operandList.add(value);
     return ImmutableNullableList.copyOf(operandList);
   }

core/src/main/java/org/apache/calcite/sql/SqlUnpivot.java

@@ -67,10 +67,10 @@ public SqlUnpivot(SqlParserPos pos, SqlNode query, boolean includeNulls,
       SqlNodeList measureList, SqlNodeList axisList, SqlNodeList inList) {
     super(pos);
     this.query = requireNonNull(query, "query");
-    this.includeNulls = includeNulls;
     this.measureList = requireNonNull(measureList, "measureList");
     this.axisList = requireNonNull(axisList, "axisList");
     this.inList = requireNonNull(inList, "inList");
+    this.includeNulls = includeNulls;
   }
 
   //~ Methods ----------------------------------------------------------------
@@ -80,7 +80,8 @@ public SqlUnpivot(SqlParserPos pos, SqlNode query, boolean includeNulls,
   }
 
   @Override public List<SqlNode> getOperandList() {
-    return ImmutableNullableList.of(query, measureList, axisList, inList);
+    return ImmutableNullableList.of(query, measureList, axisList, inList,
+        SqlLiteral.createBoolean(includeNulls, SqlParserPos.ZERO));
   }
 
   @SuppressWarnings("nullness")
@@ -176,5 +177,16 @@ static class Operator extends SqlSpecialOperator {
     Operator(SqlKind kind) {
       super(kind.name(), kind);
     }
+
+    @Override public SqlCall createCall(
+        @Nullable SqlLiteral functionQualifier,
+        SqlParserPos pos,
+        @Nullable SqlNode... operands) {
+      return new SqlUnpivot(pos, requireNonNull(operands[0]),
+          requireNonNull((SqlLiteral) operands[4]).booleanValue(),
+          requireNonNull((SqlNodeList) operands[1]),
+          requireNonNull((SqlNodeList) operands[2]),
+          requireNonNull((SqlNodeList) operands[3]));
+    }
   }
 }

core/src/main/java/org/apache/calcite/sql/ddl/SqlCreateTable.java

@@ -47,24 +47,30 @@ public class SqlCreateTable extends SqlCreate {
       new SqlSpecialOperator("CREATE TABLE", SqlKind.CREATE_TABLE) {
         @Override public SqlCall createCall(@Nullable SqlLiteral functionQualifier,
             SqlParserPos pos, @Nullable SqlNode... operands) {
-          return new SqlCreateTable(pos,
+          return new SqlCreateTable(OPERATOR, pos,
               ((SqlLiteral) requireNonNull(operands[0], "replace")).booleanValue(),
               ((SqlLiteral) requireNonNull(operands[1], "ifNotExists")).booleanValue(),
               (SqlIdentifier) requireNonNull(operands[2], "name"),
-              (SqlNodeList) requireNonNull(operands[3], "columnList"),
-              operands[4]);
+              (SqlNodeList) operands[3], operands[4]);
         }
       };
 
   /** Creates a SqlCreateTable. */
-  protected SqlCreateTable(SqlParserPos pos, boolean replace, boolean ifNotExists,
-      SqlIdentifier name, @Nullable SqlNodeList columnList, @Nullable SqlNode query) {
-    super(OPERATOR, pos, replace, ifNotExists);
+  protected SqlCreateTable(SqlOperator operator, SqlParserPos pos, boolean replace,
+      boolean ifNotExists, SqlIdentifier name, @Nullable SqlNodeList columnList,
+      @Nullable SqlNode query) {
+    super(operator, pos, replace, ifNotExists);
     this.name = requireNonNull(name, "name");
     this.columnList = columnList; // may be null
     this.query = query; // for "CREATE TABLE ... AS query"; may be null
   }
 
+  /** Creates a SqlCreateTable. */
+  protected SqlCreateTable(SqlParserPos pos, boolean replace, boolean ifNotExists,
+      SqlIdentifier name, @Nullable SqlNodeList columnList, @Nullable SqlNode query) {
+    this(OPERATOR, pos, replace, ifNotExists, name, columnList, query);
+  }
+
   @SuppressWarnings("nullness")
   @Override public List<SqlNode> getOperandList() {
     return ImmutableNullableList.of(

core/src/main/java/org/apache/calcite/sql/ddl/SqlCreateView.java

@@ -50,8 +50,8 @@ public class SqlCreateView extends SqlCreate {
           return new SqlCreateView(pos,
               ((SqlLiteral) requireNonNull(operands[0], "replace")).booleanValue(),
               (SqlIdentifier) requireNonNull(operands[1], "name"),
-              (SqlNodeList) operands[3],
-              requireNonNull(operands[4], "query"));
+              (SqlNodeList) operands[2],
+              requireNonNull(operands[3], "query"));
         }
       };
 

core/src/main/java/org/apache/calcite/sql/fun/SqlBasicOperator.java

@@ -40,24 +40,29 @@ public final class SqlBasicOperator extends SqlOperator {
   }
 
   /** Private constructor. Use {@link #create}. */
-  private SqlBasicOperator(String name, int leftPrecedence, int rightPrecedence,
+  private SqlBasicOperator(String name, SqlKind kind, int leftPrecedence, int rightPrecedence,
       SqlCallFactory callFactory) {
-    super(name, SqlKind.OTHER, leftPrecedence, rightPrecedence,
+    super(name, kind, leftPrecedence, rightPrecedence,
         ReturnTypes.BOOLEAN, InferTypes.RETURN_TYPE, OperandTypes.ANY, callFactory);
   }
 
   public static SqlBasicOperator create(String name) {
-    return new SqlBasicOperator(name, 0, 0,
+    return new SqlBasicOperator(name, SqlKind.OTHER, 0, 0,
+        SqlCallFactories.SQL_BASIC_CALL_FACTORY);
+  }
+
+  public static SqlBasicOperator create(String name, SqlKind kind) {
+    return new SqlBasicOperator(name, kind, 0, 0,
         SqlCallFactories.SQL_BASIC_CALL_FACTORY);
   }
 
   public SqlBasicOperator withPrecedence(int prec, boolean leftAssoc) {
-    return new SqlBasicOperator(getName(), leftPrec(prec, leftAssoc),
+    return new SqlBasicOperator(getName(), getKind(), leftPrec(prec, leftAssoc),
         rightPrec(prec, leftAssoc), getSqlCallFactory());
   }
 
   public SqlBasicOperator withCallFactory(SqlCallFactory sqlCallFactory) {
-    return new SqlBasicOperator(getName(), getLeftPrec(),
+    return new SqlBasicOperator(getName(), getKind(), getLeftPrec(),
         getRightPrec(), sqlCallFactory);
   }
 }

site/_docs/history.md

@@ -49,6 +49,12 @@ other software versions as specified in gradle.properties.
 #### Breaking Changes
 {: #breaking-1-42-0}
 
+* [<a href="https://issues.apache.org/jira/browse/CALCITE-7301">CALCITE-7301</a>]
+Prior to this change, most `SqlNode`s in the `org.apache.calcite.sql.ddl` package could not be unparsed
+when created with `SqlOperator#createCall`. To fix this, those `SqlNode`s now implement their own `SqlOperator`.
+`SqlNode#getOperandList()` now returns all operands required by these operators; the number and order may differ from before.
+The same applies to `SqlBabelCreateTable` and `SqlUnpivot`.
+
 * [<a href="https://issues.apache.org/jira/browse/CALCITE-6942">CALCITE-6942</a>]
 Rename the method `decorrelateFetchOneSort` to `decorrelateSortWithRowNumber`.
 

testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java

@@ -9886,6 +9886,17 @@ static void checkList(SqlNodeList sqlNodeList,
       }
     }
 
+    static SqlNode deepCopy(SqlNode sqlNode) {
+      return sqlNode.accept(new SqlShuttle() {
+        @Override public @Nullable SqlNode visit(final SqlCall call) {
+          // Handler always creates a new copy of 'call'
+          CallCopyingArgHandler argHandler = new CallCopyingArgHandler(call, true);
+          call.getOperator().acceptCall(this, call, false, argHandler);
+          return argHandler.result();
+        }
+      });
+    }
+
     @Override public void checkList(SqlTestFactory factory, StringAndPos sap,
         @Nullable SqlDialect dialect, UnaryOperator<String> converter,
         List<String> expected) {
@@ -9915,6 +9926,12 @@ static void checkList(SqlNodeList sqlNodeList,
       final Random random = new Random();
       final String sql3 = toSqlString(sqlNodeList, randomize(random));
       assertThat(sql3, notNullValue());
+
+      // Make a deep copy of the SqlNodeList, unparse it.
+      final SqlNodeList sqlNodeList3 = (SqlNodeList) deepCopy(sqlNodeList);
+      final String sql4 = toSqlString(sqlNodeList3, simple());
+      // Should be the same as we started with.
+      assertThat(sql4, is(sql1));
     }
 
     @Override public void check(SqlTestFactory factory, StringAndPos sap,
@@ -9959,6 +9976,11 @@ static void checkList(SqlNodeList sqlNodeList,
           parseStmtAndHandleEx(factory2, sql1, parser -> { });
       final String sql4 = sqlNode4.toSqlString(simple()).getSql();
       assertThat(sql4, is(sql1));
+
+      // Make a deep copy of the original SqlNode, unparse it.
+      final SqlNode sqlNode5 = deepCopy(sqlNode);
+      final String actual5 = sqlNode5.toSqlString(writerTransform).getSql();
+      assertThat(converter.apply(actual5), is(expected));
     }
 
     @Override public void checkExp(SqlTestFactory factory, StringAndPos sap,