-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Timezone aware extract SQL expression #18990
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
base: main
Are you sure you want to change the base?
Conversation
| pub mod to_timestamp; | ||
| pub mod to_unixtime; | ||
|
|
||
| // Adjusts a timestamp to local time by applying the timezone offset. |
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.
adjust_to_local_time refactored to mod.rs as a common helper function for both date_part and to_local_time
CC: @martin-g
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.
Might want to put in common.rs instead.
| EXTRACT(SECOND FROM TIMESTAMP '2024-01-01 03:05:59+08:45'); | ||
| ---- | ||
| 2023 12 31 18 20 59 | ||
|
|
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.
tested with postive and negative offset timezones such as Asia/Kolkata and America/New_York
|
Really sorry that the previous PR was closed because I forgot to BUT HOWEVER:
|
|
thank you for the improvements! @martin-g |
|
I'll try and do a full review of this today or tomorrow, thanks for the work on it |
|
Apologies on the review delay though I hope to get to it in the next few days - honest! |
| } | ||
|
|
||
| fn is_epoch(part: &str) -> bool { | ||
| let part = part_normalization(part); |
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 work seems to be done twice - once before this function is called and once in the function. Perhaps we should just remove this part_normalization() call and pass the result of the previous call on line 209.
| pub mod to_timestamp; | ||
| pub mod to_unixtime; | ||
|
|
||
| // Adjusts a timestamp to local time by applying the timezone offset. |
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.
Might want to put in common.rs instead.
d38d0b2 to
ad2beef
Compare
|
I believe this is ready to be merged, thanks! I'll try and find a committer to do the last review and put it into the merge queue. |
Which issue does this PR close?
Closes #18228
What changes are included in this PR?
timezone config options support and subsequent results produced aware of the set timezone
Are these changes tested?
slt
Are there any user-facing changes?
yes