xtern commented on PR #3966:
URL: https://github.com/apache/calcite/pull/3966#issuecomment-2357982325
> > @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 under
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]])
```
`sourceExpressionList` contains the original SCALAR_QUERY, and this looks
like an issue that needs to be fixed before merging this PR :pensive:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
