[CALCITE-6570] Add SCALAR_QUERY to sourceExpressionList of SqlUpdate

[xtern]

Validation currently fails with an AssertionError on an UPDATE query with a sub-query that requires an implicit type cast. (See CALCITE-6570 for details).

Example

update emp set empno = (
    select cast(empno as BIGINT) from emp
)

During validation, when registering subqueries, Calcite wraps the sub-query in a SCALAR_QUERY call in selectList (sqlUpdate.getSourceSelect().getSelectList()).

But after that, TypeCoersion tries to coerce types for expressions in sourceExpressionList (see TypeCoersionImpl.coerceSourceRowType()), but it contains SqlSelect instead of SCALAR_QUERY call.

As a solution I added SCALAR_QUERY to sourceExpressionList as well.

[mihaibudiu]

I wonder whether there are other statements that would need this.

[xtern]

Thanks, at least MERGE suffers from the same issue.
Added the corresponding test and fix.

[sonarqubecloud]

Quality Gate passed

Issues
5 New issues
0 Accepted issues

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

See analysis details on SonarCloud

[xtern]

@mihaibudiu, @caicancai,
we found one issue with this patch that needs attention.
After this fix, SCALAR_QUERY is present in sourceExpressionList of original SqlUpdate AST node.
The SqlToRelConverter test uses the deprecated withExpand(true) config setting, and as a result, SCALAR_QUERY is replaced to a constant.
And that's ok, but when using withExpand(false) this SCALAR_QUERY will not be replaced with a constant even at planning phase (may be the problem in SubQueryRemoveRule or somewhere else).
As a result, we can observe in the "physical" plan the original SCALAR_SUBQUERY (with LOGICAL nodes) in the sourceExpressionList of TableModify 🤔

If the patch needs to be improved, then feel free to remove it from the current scope (1.38.0), so that it doesn't delay the release.

[caicancai]

@mihaibudiu, @caicancai, we found one issue with this patch that needs attention. After this fix, SCALAR_QUERY is present in sourceExpressionList of original SqlUpdate AST node. The SqlToRelConverter test uses the deprecated withExpand(false) config setting, and as a result, SCALAR_QUERY is replaced to a constant. And that's ok, but when using withExpand(true) this SCALAR_QUERY will not be replaced with a constant even at planning phase (may be the problem in SubQueryRemoveRule or somewhere else). As a result, we can observe in the "physical" plan the original SCALAR_SUBQUERY (with LOGICAL nodes) in the sourceExpressionList of TableModify 🤔

If the patch needs to be improved, then feel free to remove it from the current scope (1.38.0), so that it doesn't delay the release.

@xtern Hi. You mean that it can only be recognized as a constant in the physical stage? It feels strange.

[xtern]

@mihaibudiu, @caicancai, we found one issue with this patch that needs attention. After this fix, SCALAR_QUERY is present in sourceExpressionList of original SqlUpdate AST node. The SqlToRelConverter test uses the deprecated withExpand(false) config setting, and as a result, SCALAR_QUERY is replaced to a constant. And that's ok, but when using withExpand(true) this SCALAR_QUERY will not be replaced with a constant even at planning phase (may be the problem in SubQueryRemoveRule or somewhere else). As a result, we can observe in the "physical" plan the original SCALAR_SUBQUERY (with LOGICAL nodes) in the sourceExpressionList of TableModify 🤔
If the patch needs to be improved, then feel free to remove it from the current scope (1.38.0), so that it doesn't delay the release.

@xtern Hi. You mean that it can only be recognized as a constant in the physical stage? It feels strange.

In my previous message I mixed up the withExpand flag values .I fixed the message.
By default the isExpand() = false, but in SqlToRelConverterTest uses isExpand() = true

Let's turn it off and see the difference in plan tree in existing test testUpdateSubQuery (it's not new test)

Without this fix the plan is:

LogicalTableModify(table=[[CATALOG, SALES, EMP]], operation=[UPDATE], updateColumnList=[[EMPNO]], sourceExpressionList=[[$0]], flattened=[true])
  LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8], EXPR$0=[$SCALAR_QUERY({
LogicalAggregate(group=[{}], EXPR$0=[MIN($0)])
  LogicalProject(EMPNO=[$0])
    LogicalFilter(condition=[=($7, $cor1.DEPTNO)])
      LogicalTableScan(table=[[CATALOG, SALES, EMP]])
})])
    LogicalTableScan(table=[[CATALOG, SALES, EMP]])

Note that sourceExpressionList=[[$0]], and SCALAR_QUERY resides only in additional logical project.

But after applying this patch the plan is:

LogicalTableModify(table=[[CATALOG, SALES, EMP]], operation=[UPDATE], updateColumnList=[[EMPNO]], sourceExpressionList=[[$SCALAR_QUERY({
LogicalAggregate(group=[{}], EXPR$0=[MIN($0)])
  LogicalProject(EMPNO=[$0])
    LogicalFilter(condition=[=($7, $cor1.DEPTNO)])
      LogicalTableScan(table=[[CATALOG, SALES, EMP]])
})]], flattened=[true])
  LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], SLACKER=[$8], EXPR$0=[$SCALAR_QUERY({
LogicalAggregate(group=[{}], EXPR$0=[MIN($0)])
  LogicalProject(EMPNO=[$0])
    LogicalFilter(condition=[=($7, $cor0.DEPTNO)])
      LogicalTableScan(table=[[CATALOG, SALES, EMP]])
})])
    LogicalTableScan(table=[[CATALOG, SALES, EMP]])

Note that

sourceExpressionList=[[$SCALAR_QUERY({
LogicalAggregate(group=[{}], EXPR$0=[MIN($0)])
  LogicalProject(EMPNO=[$0])
    LogicalFilter(condition=[=($7, $cor1.DEPTNO)])
      LogicalTableScan(table=[[CATALOG, SALES, EMP]])
})]]

and this looks like an issue that needs to be fixed before merging this PR 😔

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.