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

dssysolyatin · 2025-11-19T12:25:46Z

No description provided.

dssysolyatin · 2025-11-19T12:29:01Z

saw a useful comment from Julian on one of previous task:

It might be helpful for SqlUnparserTest to unparse everything before and after cloning. Could you enable that in this PR? Then you might not need a specific test case.

It would be good to add this to the current PR. Then we don't need such tests, I hope

xiedeyantu · 2025-11-19T13:41:35Z

So, is your PR intended to accomplish what Julian mentioned?

dssysolyatin · 2025-11-19T22:19:56Z

I added a test, but many operators in server package (ServerUnParserTest test) don’t have createCall. I’ll look into that next week.

Also, I’m not sure if I should add the operator at the end of getOperandList, or if I can reorder them to match the constructor parameter order. The last one would be much better. One example of this is UNPIVOT

dssysolyatin

Adding a common test revealed several issues (most of them were related to org.apache.calcite.sql.ddl SqlNodes). I tried to fix them in the most straightforward way

@mihaibudiu please re-review PR, it has changed quite a bit since your last approval

dssysolyatin · 2025-11-23T20:22:24Z

babel/src/main/codegen/includes/parserImpls.ftl

    ]
    {
        list.add(SqlDdlNodes.column(s.add(id).end(this), id,
-            type.withNullable(nullable), null, null));


It was added because SqlDdlNodes.column does not expect null for ColumnStrategy.

dssysolyatin · 2025-11-23T20:25:44Z

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

+  @SuppressWarnings("nullness")
+  @Override public List<SqlNode> getOperandList() {
+    return ImmutableNullableList.of(SqlLiteral.createBoolean(getReplace(), pos),
+        SqlLiteral.createSymbol(tableCollectionType, pos),


I’m taking the most straightforward approach using SqlLiteral for boolean flag for replace and ifNotExists. Ideally, replace and ifNotExists should each have their own SqlNode implementation.
That way, we won’t need to write code like this every time:

if (ifNotExists) { writer.keyword("IF NOT EXISTS"); }

Instead, we could just call:

ifNotExists.unparse(...)

dssysolyatin · 2025-11-23T20:29:03Z

site/_docs/history.md

 #### Breaking Changes
 {: #breaking-1-42-0}

+* [<a href="https://issues.apache.org/jira/browse/CALCITE-7301">CALCITE-7301</a>]


It is a breaking change, but I hope it should not affect anyone. All SqlNode classes inside org.apache.calcite.sql.ddl expose their operands through public fields, and none of these nodes implement setOperand. Because of that, I expect that getOperandList() is not used outside the Calcite codebase for these SqlNode types.

For SqlUnpivot, I added the SqlLiteral at the end of the operand list.

@mihaibudiu I personally would prefer to include this as a breaking change, including the changes you made in your ddl related PR, which you submitted separately.

The number of arguments in the operator list has changed, and this is a runtime breaking change. Existing code may assume different behavior, for example:

if (!operandList.isEmpty) { // show user first operand or another crazy cases }

sonarqubecloud · 2025-11-23T21:56:51Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
95.0% Coverage on New Code
1.1% Duplication on New Code

See analysis details on SonarQube Cloud

mihaibudiu

These changes look useful, but I think there should be a new JIRA issue for all the DDL stuff.
Can you please break down this PR into separate pieces?
Perhaps you can do all DDL in one issue.

mihaibudiu · 2025-11-24T18:52:26Z

babel/src/main/codegen/includes/parserImpls.ftl

    {
        list.add(SqlDdlNodes.column(s.add(id).end(this), id,
-            type.withNullable(nullable), null, null));
+            type.withNullable(nullable), null, ColumnStrategy.DEFAULT));


what is the relationship of this change with the issue that is being addressed?

mihaibudiu · 2025-11-24T18:53:31Z

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

-    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.


does this require filing a new issue?
if so, perhaps you can add here the issue link.

I don’t see a reason to spend time on it, as this is a very specific case.

Additionally, I think almost nobody uses this syntax. I originally introduced it while working on the implementation of the Postgres wire protocol and integrating various tools on top of it. At that time, I didn’t observe much interest in this feature outside the company I was working for

mihaibudiu · 2025-11-24T18:55:36Z

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

 public class SqlAttributeDefinition extends SqlCall {
-  private static final SqlSpecialOperator OPERATOR =
-      new SqlSpecialOperator("ATTRIBUTE_DEF", SqlKind.ATTRIBUTE_DEF);
+  private static final SqlOperator OPERATOR =


is this in scope for the current issue?

mihaibudiu · 2025-11-24T18:56:23Z

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

 public class SqlCheckConstraint extends SqlCall {
-  private static final SqlSpecialOperator OPERATOR =
-      new SqlSpecialOperator("CHECK", SqlKind.CHECK);
+  private static final SqlOperator OPERATOR =


I can sense some scope creep; you started with lambda and now you are doing many other things.
This is not even part of the list of operators in the JIRA issue.
why not do them in separate PRs?

The main reason is that I added the common test (see SqlParserTest), and after that the unparse test for all these operators started failing. I don’t think it makes sense to create a smaller PR for this. What I can do is rename the task

sure, please rename the task and then ask for a review again

I have a small suggestion—not sure if it’s acceptable. Would it be possible to create a new JIRA to work on modifying the testing framework first, and then come back to complete the current PR? That might make things clearer.

mihaibudiu · 2025-12-04T01:16:04Z

What is the status of this PR?

dssysolyatin · 2025-12-06T11:47:26Z

I will most likely come back to this PR at the end of next week

sonarqubecloud · 2025-12-28T21:13:07Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
97.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

dssysolyatin

The task was renamed, and the scope became much smaller after @mihaibudiu merged the DDL PR (Thanks again for that)

But it looks like we missed two things related DDL. See

SqlCreateView.java (wrong order of operands)
SqlCreateTable.java (columnList can be empty if syntax like CREATE TABLE t AS SELECT is used)

dssysolyatin · 2025-12-28T20:21:04Z

site/_docs/history.md

 #### Breaking Changes
 {: #breaking-1-42-0}

+* [<a href="https://issues.apache.org/jira/browse/CALCITE-7301">CALCITE-7301</a>]


@mihaibudiu I personally would prefer to include this as a breaking change, including the changes you made in your ddl related PR, which you submitted separately.

The number of arguments in the operator list has changed, and this is a runtime breaking change. Existing code may assume different behavior, for example:

if (!operandList.isEmpty) { // show user first operand or another crazy cases }

dssysolyatin marked this pull request as draft November 19, 2025 12:35

dssysolyatin force-pushed the bug/CALCITE-7301 branch from 9303179 to 12ac1a8 Compare November 19, 2025 12:42

mihaibudiu approved these changes Nov 19, 2025

View reviewed changes

dssysolyatin changed the title ~~[CALCITE-7301] Support unparse special syntax when operator is LAMBDA~~ [CALCITE-7301] Support unparse special syntax when operator is LAMBDA/MERGE/UNPIVOT Nov 19, 2025

dssysolyatin force-pushed the bug/CALCITE-7301 branch 4 times, most recently from 4d0fe65 to 659f51b Compare November 23, 2025 21:34

dssysolyatin commented Nov 23, 2025

View reviewed changes

dssysolyatin marked this pull request as ready for review November 23, 2025 21:52

mihaibudiu reviewed Nov 24, 2025

View reviewed changes

dssysolyatin mentioned this pull request Dec 19, 2025

[CALCITE-7339] Most classes in SqlDdlNodes use an incorrect SqlCallFactory #4696

Merged

dssysolyatin added 3 commits December 28, 2025 21:16

[CALCITE-7301] Support unparse special syntax when operator is LAMBDA

a778f6e

Add unify test

3497c2c

Fix server SqlNode's and add unparse test to babel

2882a33

dssysolyatin changed the title ~~[CALCITE-7301] Support unparse special syntax when operator is LAMBDA/MERGE/UNPIVOT~~ [CALCITE-7301] Add common test for SqlNode unparse after deep copy and fix operators missing createCall Dec 28, 2025

dssysolyatin force-pushed the bug/CALCITE-7301 branch from 659f51b to d4a195c Compare December 28, 2025 20:04

Remove unnecessary changes

0cee375

dssysolyatin force-pushed the bug/CALCITE-7301 branch from d4a195c to 0cee375 Compare December 28, 2025 20:13

dssysolyatin added 2 commits December 28, 2025 22:33

Fix compilation

2f89ff5

Fix ddl related bugs

6f3883e

dssysolyatin commented Dec 28, 2025

View reviewed changes

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

dssysolyatin commented Nov 19, 2025

Uh oh!

dssysolyatin commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiedeyantu commented Nov 19, 2025

Uh oh!

dssysolyatin commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dssysolyatin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dssysolyatin Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Nov 23, 2025

Quality Gate passed

Uh oh!

mihaibudiu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dssysolyatin Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mihaibudiu commented Dec 4, 2025

Uh oh!

dssysolyatin commented Dec 6, 2025

Uh oh!

sonarqubecloud bot commented Dec 28, 2025

Quality Gate passed

Uh oh!

dssysolyatin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dssysolyatin commented Nov 19, 2025 •

edited

Loading

dssysolyatin commented Nov 19, 2025 •

edited

Loading

dssysolyatin Nov 23, 2025 •

edited

Loading

dssysolyatin Nov 24, 2025 •

edited

Loading