[
https://issues.apache.org/jira/browse/CALCITE-6554?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17877906#comment-17877906
]
Julian Hyde edited comment on CALCITE-6554 at 8/29/24 10:33 PM:
----------------------------------------------------------------
Looks good.
I don't recall whether the Quidem tests use this path, but you can add a Quidem
test (say to {{{}sub-query.iq{}}}) that fails before your fix and passes after
your fix, that would give extra confidence. (None of us are great at eyeballing
a plan, but if the query runs and gives correct results, that's something.)
In any case, let's get this in before 1.38.
was (Author: julianhyde):
Looks good.
I don't recall whether the Quidem tests use this path, but you can add a Quidem
test (say to {{{}sub-query.iq{}}}) that fails before your fix and passes after
your fix, that would give extra confidence. (None of us are great at eyeballing
a plan, but if the query runs and gives correct results, that's something.)
> nested correlated sub-query in aggregation does not have inner correlation
> variable bound to inner projection
> -------------------------------------------------------------------------------------------------------------
>
> Key: CALCITE-6554
> URL: https://issues.apache.org/jira/browse/CALCITE-6554
> Project: Calcite
> Issue Type: Bug
> Affects Versions: 1.37.0
> Reporter: Ian Bertolacci
> Assignee: Ian Bertolacci
> Priority: Major
> Labels: pull-request-available
> Fix For: 1.38.0
>
>
> It appears that nested correlated subqueries can (at least in aggregates)
> result in project nodes not having their variablesSet populated with the
> correct (or any) correlation variables.
> For example:
> {code:java}
> @Test void testCorrelationInProjectionWith1xNestedCorrelatedProjection() {
> final String sql =
> "select e1.empno,\n"
> + " (select sum(e2.sal +\n"
> + " (select sum(e3.sal) from emp e3 where e3.mgr = e2.empno)\n"
> + " ) from emp e2 where e2.mgr = e1.empno)\n"
> + "from emp e1";
> sql(sql).withExpand(false).withDecorrelate(false).ok();
> }
> {code}
> currently gives the following plan:
> {code:java}
> LogicalProject(variablesSet=[[$cor0]], EMPNO=[$0], EXPR$1=[$SCALAR_QUERY({
> LogicalAggregate(group=[{}], EXPR$0=[SUM($0)])
> LogicalProject($f0=[+($5, $SCALAR_QUERY({
> LogicalAggregate(group=[{}], EXPR$0=[SUM($0)])
> LogicalProject(SAL=[$5])
> LogicalFilter(condition=[=($3, $cor1.EMPNO)])
> LogicalTableScan(table=[[CATALOG, SALES, EMP]])
> }))])
> LogicalFilter(condition=[=($3, $cor0.EMPNO)])
> LogicalTableScan(table=[[CATALOG, SALES, EMP]])
> })])
> LogicalTableScan(table=[[CATALOG, SALES, EMP]])
> {code}
> Notice that cor1 is not bound to the inner query's projection nodes
> variablesSet as cor0 is to the outer query's projection node.
> This results in improper subquery removal (which was originally believed to
> be an issue with the rule itself, discussed in CALCITE-5213)
> This is because in [SqlToRelConverter.createAggImpl, it does not check for
> correlation variables after creating an intermediate
> project|https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java#L3589-L3595]
> in the same way that
> [SqlToRelConverter.convertSelectList|https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java#L4754-L4766]
> does.
> This patch fixes addresses this issue:
> {code:none}
> diff --git
> a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
> b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
> index d88daa631..5668bc825 100644
> --- a/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
> +++ b/core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java
> @@ -3587,11 +3587,26 @@ private void createAggImpl(Blackboard bb,
> final RelNode inputRel = bb.root();
> // Project the expressions required by agg and having.
> - bb.setRoot(
> - relBuilder.push(inputRel)
> - .projectNamed(preExprs.leftList(), preExprs.rightList(), false)
> - .build(),
> - false);
> + {
> + RelNode intermediateProject = relBuilder.push(inputRel)
> + .projectNamed(preExprs.leftList(), preExprs.rightList(), false)
> + .build();
> + final RelNode r2;
> + // deal with correlation
> + final CorrelationUse p = getCorrelationUse(bb, intermediateProject);
> + if (p != null) {
> + assert p.r instanceof Project;
> + // correlation variables have been normalized in p.r, we should
> use expressions
> + // in p.r instead of the original exprs
> + Project project1 = (Project) p.r;
> + r2 = relBuilder.push(bb.root())
> + .projectNamed(project1.getProjects(),
> project1.getRowType().getFieldNames(), true, ImmutableSet.of(p.id))
> + .build();
> + } else {
> + r2 = intermediateProject;
> + }
> + bb.setRoot(r2, false);
> + }
> bb.mapRootRelToFieldProjection.put(bb.root(), r.groupExprProjection);
> // REVIEW jvs 31-Oct-2007: doesn't the declaration of
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
