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

[sonarcloud]

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 😔