-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-44838][SQL][FOLLOW-UP] Fix the test for raise_error by using default type for strings #46649
Conversation
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercion.scala
Outdated
Show resolved
Hide resolved
cc @cloud-fan and @uros-db |
For a bit of more context, the test fails as below:
because it fails to add a cast from cc @gengliangwang too |
It was my understanding that this wouldn't be a problem, since this second parameter (MapType) is only used internally in Spark to raise errors |
@HyukjinKwon I believe you could just use: this would perhaps be a more uniform approach - it's what we usually do for various expressions (we offer support for AbstractMapType as well as AbstractArrayType) |
0725ef9
to
0ad1a87
Compare
Sure, that sounds like more localized fix |
Seems like that still doesn't work with ANSI disabled:
|
im gonna switch back to just fix ANSI disabeld type coercion rule but I think we should revisit those whole thing .. |
This reverts commit 0ad1a87.
@@ -1048,6 +1048,9 @@ object TypeCoercion extends TypeCoercionBase { | |||
} | |||
} | |||
|
|||
// Allows the cast between different collated strings to match with ANSI behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a behavior change.
@uros-db I looked at the code base, we have implicit cast rules for AbstractArrayType
, but not AbstractMapType
, seems like a miss?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AbstractMapType(StringTypeAnyCollation, StringTypeAnyCollation)
does not work with non ANSI type coercion here. We should fix it here in any event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uros-db please open a separate PR - I can close mine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on it
Gentle ping, @uros-db . The CI is still broken. |
fix should be ready #46661 |
What changes were proposed in this pull request?
This PR is a followup of #46461 that fixes the CI failure when ANSI is off:
Why are the changes needed?
CI is broken https://github.com/apache/spark/actions/runs/9136253329
Does this PR introduce any user-facing change?
No, the main change has not been released out.
How was this patch tested?
Manually ran the test with ANSI disabled.
Was this patch authored or co-authored using generative AI tooling?
No.