-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: to_time function
#19540
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
feat: to_time function
#19540
Conversation
409e183 to
118f1ed
Compare
118f1ed to
9917de9
Compare
|
Thank you for this PR. I would like to see some additional slt tests
Support timestamp as input, extract time from it. |
Sure, I will add those. Thanks for feedback. |
- Extract magic numbers to NANOS_PER_SECOND, NANOS_PER_MINUTE, NANOS_PER_HOUR constants - Use Arrow's NANOSECONDS constant as base - Add timestamp input support (extracts time portion) - Add SLT tests for: - HH:MM default parsing (no seconds) - Out of range minutes/seconds - Timestamp input (various timestamp types) - Null timestamp handling
| query D | ||
| select to_time(to_timestamp('2024-01-15T14:30:45-05:00')); | ||
| ---- | ||
| 19:30:45 | ||
|
|
||
| # Timezone test using timestamp value directly | ||
| # The timestamp is converted from -05:00 to UTC, so 14:30:45-05:00 = 19:30:45 UTC | ||
|
|
||
| query D | ||
| select to_time('2024-03-20 09:15:30'::timestamp AT TIME ZONE 'America/Los_Angeles'); | ||
| ---- | ||
| 09:15:30 |
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.
These look a little odd to me; so if we specify a timestamp with an offset like -05:00 then it returns the UTC time, but if we use AT TIME ZONE it returns the local time for America/Los_Angeles? Am I missing something here, or is it inconsistent?
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.
Should return time as is without AT TIME ZONE.
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.
Sorry I can't understand what the reply means here in relation to my question
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.
What I should have explained was that yes the behavior appears inconsistent bu the inconsistency comes from how DataFusion creates timestamps, not from the new to_time itself.
to_timestamp()with offset parses and converts toUTC→ stores19:30:45::timestamp AT TIME ZONEcreates a naive timestamp first, then just labels it → stores09:15:30-07:00(same time value, different metadata)
to_time behaves consistently, it extracts whatever time value it receives. The different results come from the timestamp creation step.
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.
Thanks for the clarification. Could we retain the test that had a timestamp with timezone then, for coverage? Since to_timestamp('2024-01-15T14:30:45-05:00') doesn't actually have a timezone
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.
Done
|
Thanks @kumarUjjawal & @Omega359 |
Which issue does this PR close?
Rationale for this change
Previously we needed to create timestamp and then extract the time component from that.
What changes are included in this PR?
to_timeAre these changes tested?
Are there any user-facing changes?