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/06/02 11:30:24 UTC
[PR] [SPARK-52354] Add type coercion to UnionLoop [spark]
Pajaraja opened a new pull request, #51063:
URL: https://github.com/apache/spark/pull/51063
### What changes were proposed in this pull request?
Add a new rule to WidenSetOperationTypes to work for UnionLoop.
### Why are the changes needed?
To have UNION ALL in recursive CTEs behave in the same way regarding type coercion as UNION ALL outside the context of rCTEs.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
New Golden file tests.
### 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-52354][SQL] Add type coercion to UnionLoop [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #51063: [SPARK-52354][SQL] Add type coercion to UnionLoop
URL: https://github.com/apache/spark/pull/51063
--
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-52354][SQL] Add type coercion to UnionLoop [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #51063:
URL: https://github.com/apache/spark/pull/51063#issuecomment-2996619518
@Pajaraja can you resolve merge conflicts?
--
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-52354] Add type coercion to UnionLoop [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51063:
URL: https://github.com/apache/spark/pull/51063#discussion_r2132532111
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionBase.scala:
##########
@@ -247,6 +249,31 @@ abstract class TypeCoercionBase extends TypeCoercionHelper {
val attrMapping = s.children.head.output.zip(newChildren.head.output)
s.copy(children = newChildren) -> attrMapping
}
+
+ case s: UnionLoop
+ if s.childrenResolved && s.anchor.output.length == s.recursion.output.length
+ && !s.resolved =>
+ val incompatibleAttributes = s.anchor.output.zip(s.recursion.output).filter {
Review Comment:
I think it should be simpler
```
val casts = s.recursion.output.zip(s.anchor.output.map(_.dataType)).map {
case (attr, dt) => implicitCast(attr, dt).getOrElse(attr)
}
s.copy(recursion = Project(casts, s.recursion))
```
--
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-52354] Add type coercion to UnionLoop [spark]
Posted by "Pajaraja (via GitHub)" <gi...@apache.org>.
Pajaraja commented on code in PR #51063:
URL: https://github.com/apache/spark/pull/51063#discussion_r2137921055
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionBase.scala:
##########
@@ -247,6 +249,31 @@ abstract class TypeCoercionBase extends TypeCoercionHelper {
val attrMapping = s.children.head.output.zip(newChildren.head.output)
s.copy(children = newChildren) -> attrMapping
}
+
+ case s: UnionLoop
+ if s.childrenResolved && s.anchor.output.length == s.recursion.output.length
+ && !s.resolved =>
+ val incompatibleAttributes = s.anchor.output.zip(s.recursion.output).filter {
Review Comment:
I changed it to something like this. However, this will always cast the recursion type to the anchor type, even if the recursion type is larger (for example if the anchor type is INT, and in the recursion it's cast to BIGINT, we will cast it back to INT). Should this be the expected behavior?
--
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-52354] Add type coercion to UnionLoop [spark]
Posted by "Pajaraja (via GitHub)" <gi...@apache.org>.
Pajaraja commented on code in PR #51063:
URL: https://github.com/apache/spark/pull/51063#discussion_r2139773530
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionBase.scala:
##########
@@ -247,6 +248,17 @@ abstract class TypeCoercionBase extends TypeCoercionHelper {
val attrMapping = s.children.head.output.zip(newChildren.head.output)
s.copy(children = newChildren) -> attrMapping
}
+
+ case s: UnionLoop
+ if s.childrenResolved && s.anchor.output.length == s.recursion.output.length
+ && !s.resolved =>
+ val casts = s.recursion.output.zip(s.anchor.output.map(_.dataType)).map {
+ case (attr, dt) => implicitCast(attr, dt).getOrElse(attr)
+ }
+ val projectList = casts.zip(s.anchor.output.map(_.name)).map {
+ case (attr, name) => Alias(attr, name)()
+ }
+ s.copy(recursion = Project(projectList, s.recursion)) -> Nil
Review Comment:
Since we don't change the output of the anchor, we don't change the output of the UnionLoop, so I think we don't need to do that here.
--
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-52354] Add type coercion to UnionLoop [spark]
Posted by "Pajaraja (via GitHub)" <gi...@apache.org>.
Pajaraja commented on code in PR #51063:
URL: https://github.com/apache/spark/pull/51063#discussion_r2139780035
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionBase.scala:
##########
@@ -247,6 +248,17 @@ abstract class TypeCoercionBase extends TypeCoercionHelper {
val attrMapping = s.children.head.output.zip(newChildren.head.output)
s.copy(children = newChildren) -> attrMapping
}
+
+ case s: UnionLoop
+ if s.childrenResolved && s.anchor.output.length == s.recursion.output.length
+ && !s.resolved =>
+ val casts = s.recursion.output.zip(s.anchor.output.map(_.dataType)).map {
+ case (attr, dt) => implicitCast(attr, dt).getOrElse(attr)
Review Comment:
Changed it to this, but threw an error in the case where we can't cast because otherwise we would have an infinite analysis as UnionLoop will never get resolved.
--
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-52354] Add type coercion to UnionLoop [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51063:
URL: https://github.com/apache/spark/pull/51063#discussion_r2148942350
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionBase.scala:
##########
@@ -247,6 +249,20 @@ abstract class TypeCoercionBase extends TypeCoercionHelper {
val attrMapping = s.children.head.output.zip(newChildren.head.output)
s.copy(children = newChildren) -> attrMapping
}
+
+ case s: UnionLoop
+ if s.childrenResolved && s.anchor.output.length == s.recursion.output.length
+ && !s.resolved =>
+ val projectList = s.recursion.output.zip(s.anchor.output.map(_.dataType)).map {
+ case (attr, dt) =>
+ val widerType = findWiderTypeForTwo(attr.dataType, dt)
Review Comment:
can we add some code comments to explain the reasoning here?
--
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-52354] Add type coercion to UnionLoop [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51063:
URL: https://github.com/apache/spark/pull/51063#discussion_r2138867199
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionBase.scala:
##########
@@ -247,6 +248,17 @@ abstract class TypeCoercionBase extends TypeCoercionHelper {
val attrMapping = s.children.head.output.zip(newChildren.head.output)
s.copy(children = newChildren) -> attrMapping
}
+
+ case s: UnionLoop
+ if s.childrenResolved && s.anchor.output.length == s.recursion.output.length
+ && !s.resolved =>
+ val casts = s.recursion.output.zip(s.anchor.output.map(_.dataType)).map {
+ case (attr, dt) => implicitCast(attr, dt).getOrElse(attr)
Review Comment:
@Pajaraja you made a good point about narrower type cast. I'm thinking about something like
```
val projectList = s.recursion.output.zip(s.anchor.output.map(_.dataType)).map {
case (attr, dt) =>
val widerType = findWiderTypeForTwo(attr.dataType, dt)
if (widerType.isDefined && widerType.get == dt) {
Alias(Cast(attr, dt), attr.name)()
} else {
attr
}
}
```
--
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-52354] Add type coercion to UnionLoop [spark]
Posted by "Pajaraja (via GitHub)" <gi...@apache.org>.
Pajaraja commented on code in PR #51063:
URL: https://github.com/apache/spark/pull/51063#discussion_r2161455121
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionBase.scala:
##########
@@ -247,6 +249,20 @@ abstract class TypeCoercionBase extends TypeCoercionHelper {
val attrMapping = s.children.head.output.zip(newChildren.head.output)
s.copy(children = newChildren) -> attrMapping
}
+
+ case s: UnionLoop
+ if s.childrenResolved && s.anchor.output.length == s.recursion.output.length
+ && !s.resolved =>
+ val projectList = s.recursion.output.zip(s.anchor.output.map(_.dataType)).map {
+ case (attr, dt) =>
+ val widerType = findWiderTypeForTwo(attr.dataType, dt)
Review Comment:
Added comment.
--
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-52354] Add type coercion to UnionLoop [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #51063:
URL: https://github.com/apache/spark/pull/51063#discussion_r2138862869
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionBase.scala:
##########
@@ -247,6 +248,17 @@ abstract class TypeCoercionBase extends TypeCoercionHelper {
val attrMapping = s.children.head.output.zip(newChildren.head.output)
s.copy(children = newChildren) -> attrMapping
}
+
+ case s: UnionLoop
+ if s.childrenResolved && s.anchor.output.length == s.recursion.output.length
+ && !s.resolved =>
+ val casts = s.recursion.output.zip(s.anchor.output.map(_.dataType)).map {
+ case (attr, dt) => implicitCast(attr, dt).getOrElse(attr)
+ }
+ val projectList = casts.zip(s.anchor.output.map(_.name)).map {
+ case (attr, name) => Alias(attr, name)()
+ }
+ s.copy(recursion = Project(projectList, s.recursion)) -> Nil
Review Comment:
shall we return an attr mapping like other cases?
--
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-52354][SQL] Add type coercion to UnionLoop [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #51063:
URL: https://github.com/apache/spark/pull/51063#issuecomment-3007111731
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