You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "Pajaraja (via GitHub)" <gi...@apache.org> on 2025/05/28 14:20:26 UTC
[PR] [SPARK-52311][SQL] Redefine UnionLoop output to not be duplicated if the anchor output is duplicated [spark]
Pajaraja opened a new pull request, #51041:
URL: https://github.com/apache/spark/pull/51041
### What changes were proposed in this pull request?
Redefine the output of the UnionLoop and UnionLoopRef to be with new expression IDs.
### Why are the changes needed?
Currently rCTEs don't behave properly in case when the anchor references the same column multiple times in the anchor, leading to wrong things being identified in the recursion.
For example this rCTE:
```
WITH RECURSIVE tmp(x) AS (
values (1), (2), (3), (4), (5)
), rcte(x, y) AS (
SELECT x, x FROM tmp WHERE x = 1
UNION ALL
SELECT x + 1, x FROM rcte WHERE x < 5
)
SELECT * FROM rcte;
```
Will return:
```
1 1
2 2
3 3
4 4
5 5
```
Instead of:
```
1 1
2 1
3 2
4 3
5 4
```
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
New tests in golden file cte-recursion.sql.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52311][SQL] Redefine UnionLoop output to not be duplicated if the anchor output is duplicated [spark]
Posted by "Pajaraja (via GitHub)" <gi...@apache.org>.
Pajaraja commented on code in PR #51041:
URL: https://github.com/apache/spark/pull/51041#discussion_r2113474437
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala:
##########
@@ -79,6 +79,7 @@ object ResolveWithCTE extends Rule[LogicalPlan] {
cteDef.id,
anchor,
rewriteRecursiveCTERefs(recursion, anchor, cteDef.id, None),
+ anchor.output.map(_.newInstance().exprId),
Review Comment:
just using apply in object UnionLoop doesn't work because of default arguments.
```in object UnionLoop, multiple overloaded alternatives of method apply define default arguments.```
There are other possibilities to make this work, but I don't think any one change can work without modifying something in ResolveWithCTE.scala
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52311][SQL] Redefine UnionLoop output to not be duplicated if the anchor output is duplicated [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51041:
URL: https://github.com/apache/spark/pull/51041#discussion_r2113147121
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/cteOperators.scala:
##########
@@ -34,19 +34,31 @@ import org.apache.spark.sql.internal.SQLConf
* @param id The id of the loop, inherited from [[CTERelationDef]] within which the Union lived.
* @param anchor The plan of the initial element of the loop.
* @param recursion The plan that describes the recursion with an [[UnionLoopRef]] node.
+ * @param outputAttrIds: Seq[ExprId] Details the output attributes of the node.
Review Comment:
```suggestion
* @param outputAttrIds The ids of UnionLoop's output attributes.
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52311][SQL] Redefine UnionLoop output to not be duplicated if the anchor output is duplicated [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #51041: [SPARK-52311][SQL] Redefine UnionLoop output to not be duplicated if the anchor output is duplicated
URL: https://github.com/apache/spark/pull/51041
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52311][SQL] Redefine UnionLoop output to not be duplicated if the anchor output is duplicated [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51041:
URL: https://github.com/apache/spark/pull/51041#discussion_r2113149041
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveWithCTE.scala:
##########
@@ -79,6 +79,7 @@ object ResolveWithCTE extends Rule[LogicalPlan] {
cteDef.id,
anchor,
rewriteRecursiveCTERefs(recursion, anchor, cteDef.id, None),
+ anchor.output.map(_.newInstance().exprId),
Review Comment:
nit: we can add a new `apply` method in `object UnionLoop`, which takes the same parameters as before, and pass `anchor.output.map(_.newInstance().exprId)` additionally to construct `UnionLoop`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-52311][SQL] Redefine UnionLoop output to not be duplicated if the anchor output is duplicated [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #51041:
URL: https://github.com/apache/spark/pull/51041#issuecomment-2920982500
thanks, merging to master!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org