-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
expression: handle mixed offsets and names zones in CONVERT_TZ #28528
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
The CI found a broken test:
So this could probably be fixed and extended for testing this change too. |
/cc @mjonss |
aeb0352
to
8e64c21
Compare
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.
LGTM, if you have time, then I would suggest adding named time zones to the tests as well, like 'Europe/Amsterdam', 'Asia/Shanghai'.
Hmm, looks like it still has a bug, when adding one test with daylight saving time transition:
Fixed by adding:
last in the else branch of fromTzMatched. |
LGTM, please address the comments from @mjonss |
I would also recommend adding unit test equivalents of the following SQL clauses, if possible:
|
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.
LGTM, but I'd prefer the more uniform approach using time.FixedZone
, so that a the conversion code itself always work on two *time.Location
s, and doesn't need many if
conditions.
- Support mixed named timezones and UTC offsets - Add support for +14:00 Co-authored-by: kennytm <[email protected]>
b57784c
to
13427c0
Compare
Suggested by @kennytm
13427c0
to
c80423e
Compare
/assign @kennytm |
So looks like the expected string is wrong somehow. |
3b4a102
to
a2107ee
Compare
/merge |
This pull request has been accepted and is ready to merge. Commit hash: a2107ee
|
/label needs-cherry-pick-5.2 |
Signed-off-by: ti-srebot <[email protected]>
cherry pick to release-5.2 in PR #29311 |
Signed-off-by: ti-srebot <[email protected]>
cherry pick to release-5.3 in PR #32501 |
SYSTEM
timezoneWhat problem does this PR solve?
This fixes the issues as reported in #8311
This fixes mixing named timezones like
Europe/Moscow
and UTC offsets like+01:00
.It also fixes support for the
+14:00
timezone.Check List
Tests
Documentation
Release note