[FLINK-38702][table] Make `unparse` more reusable in parser #27256

snuyanzin · 2025-11-20T13:29:07Z

What is the purpose of the change

So far there are lots of code duplications in terms of unparse method in parser
The PR is addressing this

Brief change log

More code reuse in parser (especially for unparse)

Verifying this change

Existing parser/planner tests

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): ( no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? ( no)
If yes, how is the feature documented? (not applicable)

snuyanzin · 2025-11-20T13:29:44Z

flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl

    SqlIdentifier catalogName;
    SqlNodeList propertyList = SqlNodeList.EMPTY;
-    SqlNode comment = null;
+    SqlCharStringLiteral comment = null;


so far there were different approaches for comments
the PR aligns them

snuyanzin · 2025-11-20T13:32:09Z

...k-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlAlterTableAdd.java

+        super.unparseAlterOperation(writer, leftPrec, rightPrec);
        writer.keyword("ADD");
        // unparse table schema and distribution
        unparseSchemaAndDistribution(writer, leftPrec, rightPrec);


distribution will be moved out of schema in following PR

flinkbot · 2025-11-20T13:33:02Z

CI report:

bc64611 UNKNOWN
16035ef Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

snuyanzin · 2025-11-20T13:33:05Z

...-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlCreateDatabase.java

-
-    public String[] fullDatabaseName() {
-        return databaseName.names.toArray(new String[0]);
+    protected String getScope() {


scope is used by Calcite's SqlCreate/SqlAlter in unparse

snuyanzin · 2025-11-20T13:39:22Z

...table/flink-sql-parser/src/test/java/org/apache/flink/sql/parser/FlinkSqlParserImplTest.java

                        + "c1"
                        + commentClause
-                        + " WITH (\n"
+                        + "\nWITH (\n"


It also aligns behavior for WITH (always put on a new line)
However as a note: it doesn't impact SHOW CREATE output since for SHOW CREATE there is separate logic in ShowCreateUtil

davidradl · 2025-11-21T09:55:53Z

...k-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlAddPartitions.java

        return operands;
    }

    @Override


We should remove the @Override as well as the method. This occurs else where also

why?

it is related to unparseAlterOperation which is also overriden here
for more details have a look at https://github.com/apache/calcite/blob/49fa3edbb98e3f01fccc028c517e8abf78c1f49f/core/src/main/java/org/apache/calcite/sql/SqlAlter.java#L51

davidradl · 2025-11-21T10:14:54Z

flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlAlterModel.java

    public SqlAlterModel(SqlParserPos pos, SqlIdentifier modelName, boolean ifModelExists) {
-        super(pos);
-        this.modelName = requireNonNull(modelName, "modelName should not be null");
+        super(OPERATOR, pos, "MODEL", modelName);


I am curious about what the scope "MODEL" does. It only seems to be there for ALTERs and not for CREATEs.

Calcite will know that it should unparse as ALTER MODEL ...

for more details take a look at https://github.com/apache/calcite/blob/49fa3edbb98e3f01fccc028c517e8abf78c1f49f/core/src/main/java/org/apache/calcite/sql/SqlAlter.java

twalthr

Awesome refactoring @snuyanzin. I left some comments to make the cut between parser and converters cleaner.

...e/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlAlterCatalogComment.java

...ble/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlAlterCatalogReset.java

...able/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlAlterModelRename.java

twalthr · 2025-11-21T10:15:16Z

flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlDistribution.java

            @Nullable SqlNodeList bucketColumns,
-            @Nullable SqlNumericLiteral bucketCount) {
-        super(pos);
+            SqlNumericLiteral bucketCount) {


why not nullable anymore?

Oops, it should be for bucketColumns as it is always initialized in parser

flink/flink-table/flink-sql-parser/src/main/codegen/includes/parserImpls.ftl

Line 1519 in e5f53f2

SqlNodeList bucketColumns = SqlNodeList.EMPTY;

fixed

flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/SqlDropObject.java

...er/src/main/java/org/apache/flink/table/planner/operations/SqlNodeToOperationConversion.java

twalthr

LGTM

snuyanzin commented Nov 20, 2025

View reviewed changes

[FLINK-38702][table] Make unparse more reusable in parser

1f62493

snuyanzin force-pushed the flink38702 branch from 8d5bc55 to 1f62493 Compare November 20, 2025 14:44

davidradl reviewed Nov 21, 2025

View reviewed changes

twalthr reviewed Nov 21, 2025

View reviewed changes

snuyanzin added 2 commits November 21, 2025 13:56

Address feedback

bc64611

Address feedback

16035ef

twalthr approved these changes Nov 21, 2025

View reviewed changes

snuyanzin merged commit d0c9ed9 into apache:master Nov 21, 2025

github-actions bot added the community-reviewed PR has been reviewed by the community. label Nov 21, 2025

snuyanzin mentioned this pull request Nov 21, 2025

[FLINK-38260] Add parser changes for connection sql #27251

Merged

snuyanzin deleted the flink38702 branch November 25, 2025 17:58

[FLINK-38702][table] Make unparse more reusable in parser #27256

[FLINK-38702][table] Make unparse more reusable in parser #27256

Uh oh!

Conversation

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

Uh oh!

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

snuyanzin Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

snuyanzin Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

CI report:

Uh oh!

snuyanzin Nov 20, 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!

snuyanzin Nov 20, 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!

davidradl Nov 21, 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!

snuyanzin Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

davidradl Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

snuyanzin Nov 21, 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!

twalthr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

twalthr Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

snuyanzin Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

twalthr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[FLINK-38702][table] Make `unparse` more reusable in parser #27256

[FLINK-38702][table] Make `unparse` more reusable in parser #27256

snuyanzin commented Nov 20, 2025 •

edited

Loading

flinkbot commented Nov 20, 2025 •

edited

Loading

snuyanzin Nov 20, 2025 •

edited

Loading

snuyanzin Nov 20, 2025 •

edited

Loading

davidradl Nov 21, 2025 •

edited

Loading

snuyanzin Nov 21, 2025 •

edited

Loading