[FLINK-39577][table] Reuse Calcite's `COALESCE` to apply simplifications from `RexSimplify` by snuyanzin · Pull Request #28066 · apache/flink

snuyanzin · 2026-04-29T12:43:56Z

What is the purpose of the change

Based on top of [FLINK-39695][table] Outer join on nested field fails with conversion to relational algebra failed to preserve datatypes

There are multiple simplification in Calcite around COALESCE
the problem is that they are not applicable in Flink because it's custom implementation
Luckily we are in a phase where replacement is not painful
The PR is making that

Brief change log

Use Calcite's COALESCE

Verifying this change

Existing COALESCE 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)

Was generative AI tooling used to co-author this PR?

Yes (please specify the tool below)

snuyanzin · 2026-04-29T12:44:57Z

+
+    // ~ Methods ----------------------------------------------------------------
+
+    // ----- FLINK MODIFICATION BEGIN -----


In calcite's properties there is a flag to turn it off
the problem with that flag is that it turns off for all functions, not only for coalesce

Not sure I can follow. What does the flag do?

Can you say more about what this modification does?

it's a standard way of marking places in classes copied from Calcite what exactly area is changed in this classes

Sorry, I do understand that we are overriding / changing something from Calcite.

I am trying to understand what we are changing about the Calcite class.

see here https://github.com/apache/calcite/blob/b008df9a71cea5da44674c07e8599a0836ad1aad/core/src/main/java/org/apache/calcite/sql/fun/SqlCoalesceFunction.java#L75-L119

by default Calcite rewrites COALESCE with CASE ... WHEN and we don't want that

snuyanzin · 2026-04-29T12:45:53Z

- */
-@Internal
-@Value.Enclosing
-public class RemoveUnreachableCoalesceArgumentsRule


completely no need for this as we rely on Calcite's RexSimplify

snuyanzin · 2026-04-29T12:46:14Z

+        CoerceInputsRule.Config.DEFAULT
+          .withCoerceNames(false)
+          .withConsumerRelClass(classOf[LogicalUnion])
+          .toRule,
        // ensure intersect set operator have the same row type
-        new CoerceInputsRule(classOf[LogicalIntersect], false),
+        CoerceInputsRule.Config.DEFAULT
+          .withCoerceNames(false)
+          .withConsumerRelClass(classOf[LogicalIntersect])
+          .toRule,
        // ensure except set operator have the same row type
-        new CoerceInputsRule(classOf[LogicalMinus], false),
+        CoerceInputsRule.Config.DEFAULT
+          .withCoerceNames(false)
+          .withConsumerRelClass(classOf[LogicalMinus])
+          .toRule,


replace deprecated calls

snuyanzin · 2026-04-29T12:46:37Z

    <Resource name="ast">
      <![CDATA[
-LogicalProject(EXPR$0=[COALESCE($1, _UTF-16LE'-')])
+LogicalProject(EXPR$0=[$1])


Calcite was able to detect that the arg is not null

snuyanzin · 2026-04-29T12:47:02Z

    <Resource name="ast">
      <![CDATA[
-LogicalProject(field2=[$1], transactionId=[COALESCE($0.data.nested.trId)])
+LogicalProject(field2=[$1], transactionId=[CAST($0.data.nested.trId):VARCHAR(2147483647) CHARACTER SET "UTF-16LE"])


there is only one arg, no need for coalesce

flinkbot · 2026-04-29T12:48:27Z

CI report:

9fa9fcc Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

snuyanzin · 2026-04-29T12:59:55Z

    }
  }

+  def generateCoalesce(


this will also fix performance drop for coalesce
mentioned at FLINK-38468

jnh5y · 2026-04-29T15:55:15Z

+        if (hasNulls && nonNullNodes.size() == operands.size()) {
+            return SqlLiteral.createNull(pos);
+        }


I am having trouble understanding this condition.

For each operand, we either set hasNulls or add to the size of the nonNullNodes, right?

Given that, I don't understand how we this could be true. When hasNulls is true, then there's at least one operand which is not added to nonNullNodes, right?

i rewrote this part in proper way

jnh5y · 2026-04-29T17:12:23Z

    <Resource name="ast">
      <![CDATA[
-LogicalProject(EXPR$0=[COALESCE($1, _UTF-16LE'-')])
+LogicalProject(EXPR$0=[$1])


Since RemoveUnreachableCoalesceArgumentsRule.java has been deleted, should this file be renamed? (non-blocking nit)

I'm fin with current name it still checks for unreachable args
do you have a better name to propose?

jnh5y · 2026-04-29T17:15:18Z

+        SqlParserPos pos = call.getParserPosition();
+        List<SqlNode> nodes = new ArrayList<>();
+        for (SqlNode operand : operands) {
+            nodes.add(Objects.requireNonNullElseGet(operand, () -> SqlLiteral.createNull(pos)));


The Calcite version doesn't seem to be checking if an operand is null.

Is some path creating COALESCE(...,null,...) with a Java-null operand?

it does, however it does it for CaseCall

jnh5y · 2026-04-30T17:49:36Z

+        SqlParserPos pos = call.getParserPosition();
+        List<SqlNode> nodes = new ArrayList<>();
+        for (SqlNode operand : operands) {
+            if (!SqlUtil.isNullLiteral(operand, false)) {


Is it intentional that we are not checking for casts?

Could something interesting happen if someone trying to cast a wider type to null?

jnh5y · 2026-04-30T17:50:39Z

+
+import static java.util.Objects.requireNonNull;
+
+/** The class copied from Calcite in order to turn off COALESCE rewrite with CASE ... WHEN ... */


Do we want to add a note that we are removing the NULLs?

twalthr · 2026-04-29T12:54:44Z

+
+    // ~ Methods ----------------------------------------------------------------
+
+    // ----- FLINK MODIFICATION BEGIN -----


Not sure I can follow. What does the flag do?

twalthr · 2026-05-05T07:39:03Z

+                //   → declared DECIMAL(10, 4) (Calcite widening rule:
+                //     d = max(p1-s1, p2-s2) = max(3, 6) = 6, scale = max(2,4) = 4, precision = 10).


We shouldn't care about Calcite widening rules, but Flink ones. Defined in FlinkTypeSystem and LogicalTypeMerging.

twalthr · 2026-05-05T07:41:48Z

+        return specs.stream();
+    }
+
+    private static List<TestSetSpec> allTypesBasic() {


Please also add a restore test as a final test whether 2.3 plans (e.g. containing coalesce instead of COALESCE) work correctly.

gustavodemorais

Thanks for the change, @snuyanzin. This drops additional code and Calcite's algorithm is actually smarter, so +1.

The PR lgtm in general. Almost all test updates make sense. I just found one issue, take a look

gustavodemorais · 2026-05-05T08:27:09Z

+                        .onFieldsWithData(null, 100)
+                        .andDataTypes(INT().nullable(), INT().notNull())
+                        .withFunction(ThrowingFunction.class)
+                        .testTableApiRuntimeError(


Could you add the same negative control for sql with testSqlRuntimeError?

gustavodemorais · 2026-05-05T08:55:39Z

    <Resource name="ast">
      <![CDATA[
-LogicalProject(order_id_str=[CAST(COALESCE($3.order_id, $0)):VARCHAR(2147483647) CHARACTER SET "UTF-16LE" NOT NULL])
+LogicalProject(order_id_str=[CAST($3.order_id):VARCHAR(2147483647) CHARACTER SET "UTF-16LE" NOT NULL])


I think this is incorrect: on a LEFT JOIN with no match, $3 (= order_details_row.r) is NULL, so $3.order_id is NULL too - but the result type with the CAST still claims NOT NULL. With the original COALESCE we'd fall back to a.order_id exactly in that case

ok, finally managed to find a solution for this
submitted a separate PR #28181

I rebased on that PR, now this problem is gone

… to relational algebra failed to preserve datatypes

…ons from `RexSimplify`

snuyanzin commented Apr 29, 2026

View reviewed changes

jnh5y reviewed Apr 29, 2026

View reviewed changes

github-actions Bot added the community-reviewed PR has been reviewed by the community. label Apr 29, 2026

jnh5y reviewed Apr 29, 2026

View reviewed changes

snuyanzin force-pushed the flink39577 branch from 7d6df58 to aefcfc3 Compare April 29, 2026 19:56

jnh5y reviewed Apr 30, 2026

View reviewed changes

snuyanzin force-pushed the flink39577 branch 6 times, most recently from fdc5635 to 36c0c5d Compare May 4, 2026 07:40

twalthr reviewed May 5, 2026

View reviewed changes

gustavodemorais reviewed May 5, 2026

View reviewed changes

[FLINK-39695][table] Outer join on nested field fails with conversion…

128a6aa

… to relational algebra failed to preserve datatypes

snuyanzin mentioned this pull request May 17, 2026

[FLINK-39695][table] Outer join on nested field fails with conversion to relational algebra failed to preserve datatypes #28181

Open

1 task

snuyanzin added 4 commits May 17, 2026 13:47

[FLINK-39577][table] Reuse Calcite's COALESCE to apply simplificati…

323c73e

…ons from `RexSimplify`

[FLINK-39577][table] Add Restore test for COALESCE

34d201c

Address feedback

9bd8920

Address feedback

9fa9fcc

snuyanzin force-pushed the flink39577 branch from e23af00 to 9fa9fcc Compare May 17, 2026 12:25

rees73crs-cloud approved these changes May 17, 2026

View reviewed changes


		// ~ Methods ----------------------------------------------------------------

		// ----- FLINK MODIFICATION BEGIN -----


		import static java.util.Objects.requireNonNull;

		/** The class copied from Calcite in order to turn off COALESCE rewrite with CASE ... WHEN ... */

		// → declared DECIMAL(10, 4) (Calcite widening rule:
		// d = max(p1-s1, p2-s2) = max(3, 6) = 6, scale = max(2,4) = 4, precision = 10).

Conversation

snuyanzin commented Apr 29, 2026 • 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

Was generative AI tooling used to co-author this PR?

Uh oh!

snuyanzin Apr 29, 2026 • 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!

snuyanzin Apr 29, 2026 • 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!

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!

snuyanzin Apr 29, 2026 • 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!

flinkbot commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

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!

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!

Choose a reason for hiding this comment

Uh oh!

gustavodemorais left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

snuyanzin commented Apr 29, 2026 •

edited

Loading

snuyanzin Apr 29, 2026 •

edited

Loading

snuyanzin Apr 29, 2026 •

edited

Loading

snuyanzin Apr 29, 2026 •

edited

Loading

flinkbot commented Apr 29, 2026 •

edited

Loading

gustavodemorais left a comment •

edited

Loading