[CALCITE-7336] RelFieldTrimmer generates an incorrect plan when handl… #4691
Conversation
…ing correlated sub-query within Filter or Join
|
|
Is it possible that your proposed modifications can address this issue CALCITE-7274 ？ |
xiedeyantu
left a comment
xiedeyantu
left a comment
There was a problem hiding this comment.
LGTM! Only a small suggestion was left.
It's a related issue; I'll check it. |
| ImmutableBitSet requiredColumns = ImmutableBitSet.of(); | ||
| if (!correlationIds.isEmpty()) { | ||
| // Correlation columns are also needed by SubQueries, so add them to inputFieldsUsed. | ||
| requiredColumns = RelOptUtil.correlationColumns(correlationIds.iterator().next(), filter); |
There was a problem hiding this comment.
I'm not sure if this works correctly when nested correlations. RelOptUtil.getVariablesUsed will return all CorrelationIds except those generated within the subquery, so correlationIds may be have multiple elements. For example:
select deptno, right_q.ename from emp e1, LATERAL (
select ename from emp e2 where exists(
select * from emp e3 where e1.sal = e3.sal and e2.empno = e3.empno)) right_q
LogicalProject(DEPTNO=[$7], ENAME=[$8])
LogicalCorrelate(correlation=[$cor0], joinType=[inner], requiredColumns=[{5}])
LogicalTableScan(table=[[scott, EMP]])
LogicalProject(ENAME=[$1])
LogicalFilter(condition=[EXISTS({ <--------
LogicalFilter(condition=[AND(=($cor0.SAL, $5), =($cor1.EMPNO, $0))])
LogicalTableScan(table=[[scott, EMP]])
})], variablesSet=[[$cor1]])
LogicalTableScan(table=[[scott, EMP]])
There was a problem hiding this comment.
This is a relatively complex SQL query, and the current PR likely does not address this. Additionally, there appears to be a problem with the logical plan shown above.
LogicalProject(DEPTNO=[$7], ENAME=[$8])
LogicalCorrelate(correlation=[$cor0], joinType=[inner], requiredColumns=[{5}])
LogicalTableScan(table=[[scott, EMP]])
LogicalProject(ENAME=[$1])
LogicalFilter(condition=[EXISTS({ <--------
LogicalFilter(condition=[AND(=**($cor0.SAL, $5)**, =**($cor1.EMPNO, $0)**)])
LogicalTableScan(table=[[scott, EMP]])
})], **variablesSet=[[$cor1]]**)
LogicalTableScan(table=[[scott, EMP]])
variablesSet should include cor0 and cor1.
There was a problem hiding this comment.
If the plan is wrong it would be useful to have an issue about it.
The trimmer should convert legal plans to legal equivalent plans - otherwise there is a bug in the trimmer as well.
There was a problem hiding this comment.
variablesSet should include cor0 and cor1.
In my opinion, this plan is correct. cor1 represents a variable from the input of LogicalFilter, so it is added to variablesSet. cor0 represents the variable from the left side of LogicalCorrelate, which LogicalCorrelate has already recorded. cor0 and cor1 belong to different scopes. Adding cor0 to the variablesSet of LogicalFilter would cause errors in removing subqueries and decorrelation.
There was a problem hiding this comment.
This is a relatively complex SQL query, and the current PR likely does not address this.
This is indeed quite complex. As long as it doesn't lead to missing any columns, it's fine.
…ing correlated sub-query within Filter or Join
| // Build new filter with trimmed input and condition. | ||
| relBuilder.push(newInput) | ||
| .filter(filter.getVariablesSet(), newConditionExpr); | ||
| .filter(correlationIds, newConditionExpr); |
There was a problem hiding this comment.
Perhaps we should retain the original logic. correlationIds might contain free variables that don't belong to the current scope.
From the test cases below, it's indeed observed that this change corrects the issue of some nodes having missing variablesSet. However, the problem of missing variablesSet doesn't originate from RelFieldTrimmer; in my observation, it's usually caused by manually constructed plans. If RelNode is converted from SqlNode using SqlToRelConverter, this problem doesn't occur.
There was a problem hiding this comment.
Yes, I agree with your view. In principle, the RelNode constructed via RelNodeBuilder can be directly converted into SQL. However, I've found that some of the generated RelNodes cannot be converted. Regarding what you mentioned about the missing variablesSet—its maybe should merge these two—and the SQL test cases are also somewhat incomplete.
There was a problem hiding this comment.
Hi @silundong , You can try CALCITE-7274, Then you will know the RelNode is converted from SqlNode using SqlToRelConverter, this problem still occur.
There was a problem hiding this comment.
Because during the SqlToRelConverter process, some correlation variables from subqueries cannot be fully extracted due to scope limitations, I am considering merging the correlation variables already present in the RelNode with those newly extracted from subqueries. WDYT？
There was a problem hiding this comment.
Correlated subqueries in join condition are a tricky issue (the case in CALCITE-7274). Here's my understanding：
According to the logic in SqlToRelConverter and LogicalJoin, the variablesSet of LogicalJoin describes the variables coming from the left input that are referenced in the right input. This is also confirmed by the logic of RelBuilder constructing a join: RelBuilder.join will produce a Correlate (not a Join) when it receives a non-empty variablesSet.
However, if a subquery in the join condition uses a correlated variable (call it cor0) from the current scope, that cor0 is intended to represent the concatenation of the left and right rows, not only variable originating from the left input. That interpretation does not match the variablesSet concept in LogicalJoin, so the CorrelationId used by the subquery is not added to variablesSet. This explains why LogicalJoin.variablesSet is always empty even though the join condition contains correlated variables from the current scope. In other words, we are forced to treat free variables used by subqueries in join condition as coming from the current scope rather than from outer scopes, which also causes problems with removing nested correlated subqueries and decorrelating them.
Back to this PR: based on the above, perhaps we should retain the original logic. For Filter, its variablesSet is correct in the first place; for Join, filling the variablesSet with the correlated variables used in the subquery of the condition contradicts the definition of LogicalJoin.variablesSet.
This is my personal understanding and suggestion for your reference.
There was a problem hiding this comment.
However, if a subquery in the join condition uses a correlated variable (call it cor0) from the current scope, that cor0 is intended to represent the concatenation of the left and right rows, not only variable originating from the left input. That interpretation does not match the variablesSet concept in LogicalJoin, so the CorrelationId used by the subquery is not added to variablesSet. This explains why LogicalJoin.variablesSet is always empty even though the join condition contains correlated variables from the current scope. In other words, we are forced to treat free variables used by subqueries in join condition as coming from the current scope rather than from outer scopes, which also causes problems with removing nested correlated subqueries and decorrelating them.
Interesting, if we can transform such cases to ?
LogicalCorrelate(correlation=[$cor0])
LogicalJoin(condition=TRUE)
LogicalFilter(condition=on_condition)
Or is it illegal ?
There was a problem hiding this comment.
As @julianhyde frequently says, the root of the problem is the lack of a precise specification of the structure of Rel programs that contain correlations. In the absence of a specification there is no way to decide what is a legal program and what its meaning is. In the paragraph above @silundong has tried to write a part of a specification.
I think that this is the first step that should be made: write a spec of cor variables and variableSet, probably as a long JavaDoc comment, get it reviewed. Then write a second validator for Rel trees, which enforces these rules. That validator could be part of the builder API. Finally, once these rules are established, it will be much easier to decide where the problems lie with specific bugs.
I will file a new JIRA issue about this.
|
I have filed https://issues.apache.org/jira/browse/CALCITE-7340 |
6 participants
…ing correlated sub-query within Filter or Join