[CALCITE-7859] SqlToRelConverter.convertMerge produces a wrong plan or fails when a sub-query in MERGE ... UPDATE SET is expanded by joooger · Pull Request #5332 · apache/calcite

joooger · 2026-10-15T17:13:23Z

Jira: https://issues.apache.org/jira/browse/CALCITE-7859

SqlToRelConverter.convertMerge assumes that the input of the projection of the MERGE source select is the join of the source and the target. With expand = true a sub-query in a SET expression is expanded into a join or a correlate on top of that join, which breaks the assumption:

a correlated sub-query fails with ClassCastException: LogicalCorrelate cannot be cast to LogicalJoin;
an uncorrelated sub-query is converted into a wrong plan: the number of source fields is taken from the wrong join, so the target columns are dropped from the update row (LogicalProject(EXPR$0=[$18]) instead of the 9 target columns followed by EXPR$0).

Changes:

take the number of source fields from the validator namespace of the source table reference;
build the new projection over the real input of the source projection instead of the first join;
add testMergeUpdateCorrelatedSubQueryExpanded and testMergeUpdateUncorrelatedSubQueryExpanded.

CALCITE-7861

Jira: https://issues.apache.org/jira/browse/CALCITE-7861 (separate commit; it changes the same part of convertMerge)

With expand = true a sub-query in the VALUES of the INSERT branch of a MERGE was converted into a plan that silently inserts a wrong value:

merge into empnullables e using (select * from emp) t on e.empno = t.empno
when matched then update set sal = 1
when not matched then insert (empno, sal) values (t.empno, (select max(deptno) from dept))

SAL was EMPNO0=[$9] (EMPNO of the target) and the join with DEPT was gone. convertMerge converted the INSERT separately with convertInsert and copied the expressions of its projections into the projection over the MERGE join; when the sub-query is expanded, those expressions are over the source joined with the sub-query, and their indexes are wrong over the MERGE join.

Changes:

convert the insert values over the input of the MERGE source projection (source fields come first in it), so that their sub-queries are expanded on top of it; the target columns and the defaults are still built by convertColumnList;
create the MERGE modification through ModifiableTable.toModificationRel, as INSERT does. convertInsert used to do it as a side effect, and JdbcTable registers its convention there; without it JdbcAdapterTest.testMerge loses the JDBC push-down;
add testMergeInsertSubQuery, testMergeInsertSubQueryExpanded and testMergeInsertCorrelatedSubQueryExpanded.

Plans of the existing MERGE tests do not change. One behavior change, now pinned by testMergeModifiableView: for a MERGE into a modifiable view, the insert row used to be built for the underlying table (including the column values of the view definition), while the modification is on the view; now it is built for the view, as the update part, so the values of the columns constrained by the view are not set. MERGE into a modifiable view was not supported correctly before either; proper support (rewriting the MERGE to the underlying table, as createModify does for INSERT) is out of scope of this PR.

mihaibudiu

This PR ensures that MERGE can be compiled, but there is no proof that it would produce the right results. Is there a runtime implementation of MERGE that could be used to test the results end-to-end?

mariadb-IuriiGerzhedovich · 2026-10-16T07:54:13Z

This PR ensures that MERGE can be compiled, but there is no proof that it would produce the right results. Is there a runtime implementation of MERGE that could be used to test the results end-to-end?

@mihaibudiu
Thanks for the review. I checked what can execute a MERGE in Calcite itself:

There is no Enumerable implementation: 'EnumerableTableModify' supports only INSERT, UPDATE and DELETE and fails with 'unsupported operation: MERGE'.
The only end-to-end path is JDBC push-down ('JdbcTableModify' + 'RelToSqlConverter', executed by the external database), which 'JdbcAdapterTest.testMerge' uses with HSQLDB.

I tried the cases of this PR on that path (foodmart on HSQLDB):

An uncorrelated sub-query in SET: the plan is now correct and fully pushed to JDBC, but 'RelToSqlConverter' assumes that the input of the MERGE projection is the join of the source and the target, and generates invalid SQL ('USING (VALUES ...) AS "t" INNER JOIN "foodmart"."expense_fact" ON ... ON TRUE', rejected by HSQLDB).
A correlated sub-query: after decorrelation the 'SINGLE_VALUE' aggregate is not pushed to JDBC, so the MERGE stays in Enumerable convention and fails with unsupported operation: MERGE.

So, as far as I can see, these cases cannot be tested end-to-end in Calcite without either an Enumerable implementation of MERGE or fixing RelToSqlConverter for MERGE with sub-queries, which I think are separate issues. I can log them in Jira. In this PR the tests check the plans: the field references point to the correct source/target columns, and the existing 'JdbcAdapterTest.testMerge' keeps passing end-to-end.

mihaibudiu · 2026-10-17T04:36:06Z

I will try to review the code as it is. If you expect to have a running MERGE maybe you can file an issue.

mihaibudiu · 2026-10-19T22:14:55Z

+  /** Converts the values of the INSERT of a MERGE over the input of the MERGE
+   * source projection, whose first fields are the source fields.
+   *
+   * <p>The values are not converted over the source alone, so that their


I don't understand this paragraph. It says how things are NOT converted, but it should say how they ARE converted.

Agreed. I rewrote it to say how the values are converted: over the merge input, which is the join of the source and the target plus the joins or correlates that expand the SET sub-queries. If the sub-queries of the values are expanded, their joins or correlates are added on top of it

mihaibudiu · 2026-10-19T22:16:54Z

+   * sub-queries are expanded on top of the merge input and their field
+   * references stay valid.
+   *
+   * @return projection whose expressions are the inserted row, in the order of


A projection

mihaibudiu · 2026-10-19T22:17:28Z

+   * references stay valid.
+   *
+   * @return projection whose expressions are the inserted row, in the order of
+   * the target columns; its input is the merge input, possibly extended by


I am not sure how the merge input is "extended".
Isn't the input a query, which may or may not contain subqueries?

The merge input here is not the SQL query but the already converted RelNode: the join of the source and the target. If the sub-queries in the INSERT values are expanded, they become joins (or correlates) on top of it, so the input of the returned projection is either the merge input itself or these joins. I rewrote the @return to say this instead of "extended"

mihaibudiu · 2026-10-19T22:18:45Z

-    return LogicalTableModify.create(targetTable, catalogReader,
-        relBuilder.build(), LogicalTableModify.Operation.MERGE,
-        targetColumnNameList, null, false);
+  /** Creates the modification of a MERGE. As for INSERT, a modifiable table


I don't understand this comment.
What does this have to do with JDBC.
What convention?

JdbcTable.toModificationRel registers the rules of the JDBC convention in the planner; without them a MERGE into a JDBC table cannot be planned (JdbcAdapterTest.testMerge). Before this PR, convertMerge got that registration as a side effect of calling convertInsert, which goes through createModify. The new code no longer calls convertInsert, so createMergeModify calls toModificationRel itself, like createModify does for INSERT. A side effect is that a MERGE without an INSERT now also gets it. I rewrote the javadoc to explain this.

I think SqlToRelConverter should work without any Jdbc-related code. Moreover, there is no planning yet at this stage, there is just conversion from one representation to another.

Agreed, the javadoc was misleading. The converter itself does not depend on JDBC. Like createModify, which handles INSERT, UPDATE and DELETE, createMergeModify only calls the generic ModifiableTable.toModificationRel, so a table can create its own TableModify. What a particular table does there, JdbcTable included, is up to its adapter. Before this PR, MERGE already went through the same call via convertInsert → createModify, so this keeps the existing behavior. I removed the JDBC-specific text from the javadoc.

mihaibudiu

this generally looks fine, but the comments are misleading.
I am approving the code, but please fix the comments.

mihaibudiu · 2026-10-21T23:32:39Z

+  /** Converts the values of the INSERT of a MERGE over the input of the MERGE
+   * source projection, whose first fields are the source fields.
+   *
+   * <p>The values reference the source fields, so they are converted over the


this comment seems redundant with the returns comment. Can you remove it or trim it down?

Done, I removed the description and kept only the @return.

mihaibudiu · 2026-10-21T23:34:13Z

+   * added on top of the merge input; the source fields stay first, so the
+   * field references of the values stay valid.
+   *
+   * @return A projection whose expressions are the inserted row, in the order


moreover, if you read the body if the function it does nothing about joins, correlates, or subqueries. These are all handled by convertSelectList I suspect. So you should not mention that here. The select query is converted like any other query.

Agreed, these are details of convertSelectList. The javadoc now only says what the caller relies on: the input of the returned projection starts with the fields of mergeInput.

mihaibudiu · 2026-10-22T16:43:12Z

Please squash the commits for merging.

…r fails when a sub-query in MERGE ... UPDATE SET is expanded

joooger · 2026-10-22T17:50:27Z

Squashed into one commit.

sonarqubecloud · 2026-10-22T18:36:49Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud

joooger force-pushed the CALCITE-7859 branch 2 times, most recently from eb74259 to 9381762 Compare October 15, 2026 18:18

joooger force-pushed the CALCITE-7859 branch from 9381762 to 984819b Compare October 15, 2026 19:55

mihaibudiu reviewed Oct 15, 2026

View reviewed changes

mihaibudiu reviewed Oct 19, 2026

View reviewed changes

joooger force-pushed the CALCITE-7859 branch from 600b039 to 1a7fcd1 Compare October 21, 2026 10:57

mihaibudiu approved these changes Oct 21, 2026

View reviewed changes

joooger force-pushed the CALCITE-7859 branch from 1a7fcd1 to efcdf19 Compare October 22, 2026 04:44

mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Oct 22, 2026

[CALCITE-7859] SqlToRelConverter.convertMerge produces a wrong plan o…

0451518

…r fails when a sub-query in MERGE ... UPDATE SET is expanded

joooger force-pushed the CALCITE-7859 branch from efcdf19 to 0451518 Compare October 22, 2026 17:50

mihaibudiu merged commit 9b2bde4 into apache:main Oct 22, 2026
19 checks passed

Conversation

joooger commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mihaibudiu left a comment

Choose a reason for hiding this comment

Uh oh!

mariadb-IuriiGerzhedovich commented Oct 16, 2026

Uh oh!

mihaibudiu commented Oct 17, 2026

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!

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!

mihaibudiu commented Oct 22, 2026

Uh oh!

joooger commented Oct 22, 2026

Uh oh!

sonarqubecloud Bot commented Oct 22, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joooger commented Oct 15, 2026 •

edited

Loading