feat: Add from_datetime method to Timestamp types#9345
feat: Add from_datetime method to Timestamp types#9345codephage2020 wants to merge 15 commits intoapache:mainfrom
Conversation
codephage2020
force-pushed
the
issue-9337
branch
from
a0e087f to
6a39b1c
Compare
codephage2020
force-pushed
the
issue-9337
branch
from
6a39b1c to
617a44d
Compare
…into issue-9337
1. Introduce from_datetime<Tz: TimeZone> and a from_naive_datetime default to the ArrowTimestampType trait to provide a unified, timezone-aware way to create timestamp values. Implement these methods for all Timestamp* types . 2. Update and refactor tests to exercise UTC and FixedOffset conversions, subsecond precision, and naive-datetime-to-timezone roundtrips.
sdf-jkl
left a comment
sdf-jkl
left a comment
There was a problem hiding this comment.
Everything LGTM, but one comment below
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @codephage2020 and @sdf-jkl -- this looks great to me, except for one question (on returning Option). Let me know what you think
arrow-array/src/types.rs
Outdated
| } | ||
|
|
||
| fn from_datetime<Tz: TimeZone>(datetime: DateTime<Tz>) -> i64 { | ||
| datetime.timestamp_nanos_opt().expect("timestamp overflow") |
There was a problem hiding this comment.
🤔 this panic is unfortunate. Perhaps we should change the signature to return Option to avoid this?
There was a problem hiding this comment.
Good point. Will push an update shortly.
arrow-array/src/types.rs
Outdated
| /// # Arguments | ||
| /// | ||
| /// * `naive` - The local datetime to convert | ||
| /// * `tz` - Optional timezone. If `None`, interprets as UTC. |
There was a problem hiding this comment.
Can you also add a note here that that passing None is the same as calling Self::make_value?
And while we are considering this, I think it it would be worth deprecating make_value entirely and suggesting everyone uses from_naive_datetime (as a follow on PR, not in this one). Thoughts?
There was a problem hiding this comment.
This would make naming more intuitive, good call
There was a problem hiding this comment.
Will add a note that passing None is equivalent to calling make_value.
And +1 on deprecating make_value in a follow-up PR.
3 participants
Which issue does this PR close?
Closes #9337
Rationale for this change
What changes are included in this PR?
Are these changes tested?
YES
Are there any user-facing changes?
YES, Now it is possible to directly convert from datatime to timestamp.