Repository navigation
HIVE-27741 handle Invalid timezone value in to_utc_timestamp() UDF - #5014
Merged
Merged
Conversation
aturoczy
approved these changes
Jan 22, 2024
aturoczy
left a comment
aturoczy
left a comment
Contributor
There was a problem hiding this comment.
The change is fine. Need a test re-run and green output.
okumin
reviewed
Jan 26, 2024
asf-ci-hive
added
tests pending
tests passed
and removed
tests failed
tests pending
tests passed
labels
zratkai
commented
Feb 1, 2024
zratkai
left a comment
zratkai
left a comment
Contributor
Author
There was a problem hiding this comment.
Please check the comments and fixes.
asf-ci-hive
added
tests unstable
tests pending
and removed
tests pending
tests unstable
labels
simhadri-g
reviewed
Feb 1, 2024
simhadri-g
reviewed
Feb 1, 2024
simhadri-g
reviewed
Feb 1, 2024
simhadri-g
reviewed
Feb 1, 2024
zratkai
commented
Feb 2, 2024
zratkai
left a comment
zratkai
left a comment
Contributor
Author
There was a problem hiding this comment.
Please check my comments, changes.
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
Contributor
Author
|
@okumin @simhadri-g could you please review? |
Contributor
Author
|
@okumin @simhadri-g could you please review? Refactored to use ZoneId instead of TimeZone. |
okumin
approved these changes
Feb 9, 2024
okumin
left a comment
okumin
left a comment
Contributor
There was a problem hiding this comment.
+1 with some minor cosmetic comments.
This could be an incompatible change. But I understand the current behavior is surprising and it sounds better to throw an error(or put NULL at least). The final judgment will be done by a committer.
|
Contributor
Author
|
@okumin @simhadri-g could you please review this? |
Contributor
|
Looks good to me. Note that you finally need approval from a committer |
okumin
approved these changes
Mar 13, 2024
deniskuzZ
reviewed
Apr 8, 2024
deniskuzZ
reviewed
Apr 8, 2024
deniskuzZ
reviewed
Apr 8, 2024
deniskuzZ
reviewed
Apr 8, 2024
deniskuzZ
left a comment
deniskuzZ
left a comment
Member
There was a problem hiding this comment.
LGTM, minor code-style comments
RyanEugene
approved these changes
Apr 8, 2024
…row exception instead a default value Change-Id: I68ee1783614b4b74093df0dddb613fa860ed202a
asf-ci-hive
added
tests pending
tests unstable
and removed
tests passed
tests pending
labels
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change-Id: I68ee1783614b4b74093df0dddb613fa860ed202a
What changes were proposed in this pull request?
The to_utc_timestamp() UDF used UTC when the given timezone was invalid. It throws exception in this case to avoid data inconsistency.
Why are the changes needed?
To avoid data inconsistency in case of invalid timezone.
Does this PR introduce any user-facing change?
Yes. The to_utc_timestamp() UDF will throw exception in case of invalid timezone.
Is the change a dependency upgrade?
No.
How was this patch tested?
Unit test.