Timezone aware extract SQL expression#18990
Conversation
github-actions
bot
added
documentation
| 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.
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.
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.
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.
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.
Might want to put in common.rs instead.
codetyri0n
force-pushed
the
extract_withtz
branch
from
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. |
|
I'll try and review this in the next day or two. @codetyri0n there appears to be some conflicts with this PR -- could you please resolve them? |
hey @alamb , I've resolved the conflicts a few days back (icymi!) |
|
@codetyri0n I see there is conflicts on this PR now - would you be able to resolve those? |
codetyri0n
force-pushed
the
extract_withtz
branch
from
15bf77a to
0794846
Compare
|
Still failing ci tests ... |
|
sorry for this... ill resolve it properly first thing tomorrow |
codetyri0n
force-pushed
the
extract_withtz
branch
2 times, most recently
from
d2f0dc6 to
e80984d
Compare
|
after a series of conflict resolving clippy tells me this is unused, any idea @Omega359 ? |
codetyri0n
force-pushed
the
extract_withtz
branch
from
e80984d to
b008a2a
Compare
I'll try and take a look this week but I'm rather swamped at the moment with non-df related work :( |
| // Get the timezone offset for this datetime | ||
| let tz_offset = tz.offset_from_utc_datetime(&date_time.naive_utc()); | ||
| // Convert offset to seconds - offset is formatted like "+01:00" or "-05:00" | ||
| let offset_str = format!("{tz_offset}"); |
There was a problem hiding this comment.
When working on merging updates from main I noticed that this function in to_local_time is much simplier than this:
let offset_seconds: i64 = tz
.offset_from_utc_datetime(&date_time.naive_utc())
.fix()
.local_minus_utc() as i64;Did this function come from somewhere else or did you make this change for some other reason?
4 participants
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