Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[SPARK-49773][SQL] uncaught Java exception from make_timestamp() with bad timezone #48260

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

itholic
Copy link
Contributor

@itholic itholic commented Sep 26, 2024

What changes were proposed in this pull request?

This PR proposes to fix the uncaught Java exception from make_timestamp() with bad timezone

Why are the changes needed?

To improve the error message

Does this PR introduce any user-facing change?

No API changes, but the user-facing error message is changed:

Before

spark-sql (default)> select make_timestamp(1, 2, 28, 23, 1, 1, -100);
Invalid ID for ZoneOffset, invalid format: -100

After

spark-sql (default)> select make_timestamp(1, 2, 28, 23, 1, 1, -100);
[INVALID_ZONE_OFFSET] Invalid ID for ZoneOffset, invalid format: -100. ZoneOffset represents the time difference from UTC and must be in the range from -18:00 to +18:00. SQLSTATE: 22009

How was this patch tested?

CI

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

No

@itholic
Copy link
Contributor Author

itholic commented Sep 26, 2024

cc @srielau @cloud-fan

@github-actions github-actions bot added the SQL label Sep 26, 2024
@@ -3267,6 +3267,12 @@
},
"sqlState" : "42000"
},
"INVALID_ZONE_OFFSET": {
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we clearly say it's time zone?

@@ -3267,6 +3267,12 @@
},
"sqlState" : "42000"
},
"INVALID_ZONE_OFFSET": {
"message": [
"<message>. ZoneOffset represents the time difference from UTC and must be in the range from -18:00 to +18:00."
Copy link
Contributor

Choose a reason for hiding this comment

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

it's a bad practice to pass in the entire message. We can just say the zone offset is invalid, and we keep the original error as the causedBy of this error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why we need to preserve the caused by. Or is that merely for the stack trace?

Copy link
Contributor

Choose a reason for hiding this comment

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

yea, and also to retain the original error message from the JDK library.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants