Skip to content
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-50478][SQL] Fix StringType matching #49043

Closed
wants to merge 4 commits into from

Conversation

jovanm-db
Copy link
Contributor

@jovanm-db jovanm-db commented Dec 3, 2024

What changes were proposed in this pull request?

In canUpCast method in UpCastRule.scala we match against StringType, which does not match collated string, as opposed to matching _: StringType. Similarly, in needsTimeZone method in Cast.scala it is matched against non collated StringType.

Why are the changes needed?

Upcasting to collated strings was disabled even when upcasting to non collated strings was not. Likewise, casting from timestamp to collated string did not need time zone.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Test added in DataTypeWriteCompatibilitySuite.scala and CastSuiteBase.scala.

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the SQL label Dec 3, 2024
@@ -40,8 +40,8 @@ private[sql] object UpCastRule {
case (DateType, TimestampNTZType) => true
case (TimestampNTZType, TimestampType) => true
case (TimestampType, TimestampNTZType) => true
case (_: AtomicType, StringType) => true
case (_: CalendarIntervalType, StringType) => true
case (_: AtomicType, _: StringType) => true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add some tests for upcast here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please make sure that this does not allow casting from int to string directly by calling cast. In general by standard we should do cast (value as string) collate collation, but it is a good catch that we need to allow this transition in other operations.

Copy link
Contributor

@stefankandic stefankandic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jovanm-db jovanm-db changed the title [SPARK-50478] Allow upcasting to collated string [SPARK-50478] Fix StringType matching Dec 3, 2024
@mihailom-db
Copy link
Contributor

Please remove draft and add [SQL] component to the title of the PR.

@jovanm-db jovanm-db changed the title [SPARK-50478] Fix StringType matching [SPARK-50478][SQL] Fix StringType matching Dec 5, 2024
@jovanm-db jovanm-db marked this pull request as ready for review December 5, 2024 09:51
@MaxGekk
Copy link
Member

MaxGekk commented Dec 6, 2024

+1, LGTM. Merging to master.
Thank you, @jovanm-db and @stefankandic @mihailom-db for review.

@MaxGekk MaxGekk closed this in 1d6932c Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants