Add timezone abstraction #2909
Add timezone abstraction #2909
Conversation
| @@ -1470,23 +1470,6 @@ mod tests { | |||
| assert_eq!(array1, array2); | |||
| } | |||
|
|
|||
| #[cfg(feature = "chrono-tz")] | |||
There was a problem hiding this comment.
This test is moved to timezone.rs with some tweaks
|
|
||
| #[cfg(feature = "chrono-tz")] | ||
| #[test] | ||
| fn test_using_chrono_tz_and_utc_naive_date_time() { |
There was a problem hiding this comment.
This test is moved to timezone.rs
|
|
||
| /// Parses a fixed offset of the form "+09:00" | ||
| fn parse_fixed_offset(tz: &str) -> Result<FixedOffset, ArrowError> { | ||
| if tz.len() != 6 { |
There was a problem hiding this comment.
I'm not sure why we are so strict on this, +0900 is a valid timezone, but there was an explicit test for this so 🤷
There was a problem hiding this comment.
i think we could submit another pr to extend this to support more fixedoffset formats. chrono supports these:
There was a problem hiding this comment.
Yeah, there was just an explicit test that +0930 was not accepted, which made me think the lack of support was intentional
There was a problem hiding this comment.
just submitted a ticket, i can implement this after the pr merged
| /// Converts an [`ArrowPrimitiveType`] to [`DateTime<Tz>`] | ||
| pub fn as_datetime_with_timezone<T: ArrowPrimitiveType>( | ||
| v: i64, | ||
| tz: Tz, | ||
| ) -> Option<DateTime<Tz>> { | ||
| let naive = as_datetime::<T>(v)?; | ||
| Some(Utc.from_utc_datetime(&naive).with_timezone(&tz)) | ||
| } | ||
|
|
There was a problem hiding this comment.
this could enable displaying Timestamp with timezone 👍
|
looks great to me, thank you @tustvold |
|
the return data types for i've tried to rebase this branch from master and it worked well |
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| //! Timezone for timestamp arrays |
There was a problem hiding this comment.
I think it is worth documenting somewhere what enabling the chrono-tz feature of arrow does (and what the expected behavior differences are when it is / isn't present)
There was a problem hiding this comment.
It isn't a new feature and is already documented
| @@ -49,6 +49,7 @@ arrow-buffer = { version = "25.0.0", path = "../arrow-buffer" } | |||
| arrow-schema = { version = "25.0.0", path = "../arrow-schema" } | |||
| arrow-data = { version = "25.0.0", path = "../arrow-data" } | |||
| chrono = { version = "0.4", default-features = false, features = ["clock"] } | |||
| chrono-tz = { version = "0.7", optional = true } | |||
There was a problem hiding this comment.
I verified we added an equivalent feature to arrow/Cargo.toml so people can activate this feature from arrow
|
Benchmark runs are scheduled for baseline = 7e5d4a1 and contender = d9fd1d5. d9fd1d5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Part of #2594
Part of #599
Part of #1380
Rationale for this change
The existing logic was very tricky to understand, as it relied on applying offsets to
NaiveDateTime. This is very easy to accidental mess up, adding instead of subtracting, forgetting to apply it, etc...It also was complicating splitting up the crates, as the logic was split across multiple locations
What changes are included in this PR?
Uses the chrono timezone plumbing to provide type safe timezone APIs
Are there any user-facing changes?
No