andygrove commented on code in PR #11482:
URL: https://github.com/apache/arrow-rs/pull/11482#discussion_r4507762250


##########
arrow-array/src/timezone.rs:
##########
@@ -35,7 +77,604 @@ fn parse_fixed_offset(tz: &str) -> Option<FixedOffset> {
     }
 }
 
-#[cfg(feature = "chrono-tz")]
+#[cfg(feature = "jiff")]
+mod private {
+    use super::*;
+    use chrono::offset::TimeZone;
+    use chrono::{Datelike, LocalResult, NaiveDate, NaiveDateTime, NaiveTime, 
Offset, Timelike};
+    use jiff::Timestamp;
+    use jiff::civil::DateTime as JiffDateTime;
+    use jiff::tz::{AmbiguousOffset, TimeZone as JiffTimeZone};
+    use std::collections::HashMap;
+    use std::fmt::Display;
+    use std::str::FromStr;
+    use std::sync::OnceLock;
+
+    /// An [`Offset`] for [`Tz`]
+    #[derive(Debug, Copy, Clone)]
+    pub struct TzOffset {
+        tz: Tz,
+        offset: FixedOffset,
+    }
+
+    impl std::fmt::Display for TzOffset {
+        fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+            self.offset.fmt(f)
+        }
+    }
+
+    impl Offset for TzOffset {
+        fn fix(&self) -> FixedOffset {
+            self.offset
+        }
+    }
+
+    /// A named timezone resolved by jiff, together with its name.
+    struct NamedTz {
+        name: &'static str,
+        tz: JiffTimeZone,
+    }
+
+    /// Prints only the name, so that `Debug` output matches the `chrono-tz` 
feature.
+    impl std::fmt::Debug for NamedTz {
+        fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+            f.write_str(self.name)
+        }
+    }
+
+    /// Builds a timezone from the database in jiff-tzdb.
+    ///
+    /// Names are not resolved through `jiff::tz::db()`: if any crate in the 
build enables jiff's
+    /// `tzdb-zoneinfo` feature, that reads the host's zoneinfo files, or 
`$TZDIR`, instead. Reading
+    /// jiff-tzdb directly also means [`tzdb_version`] always describes the 
data in use.
+    fn bundled_tz(name: &str) -> Option<JiffTimeZone> {
+        // As jiff's own bundled database does, so that UTC is not backed by 
TZif data
+        if name == "UTC" {
+            return Some(JiffTimeZone::UTC);
+        }

Review Comment:
   That check is needed in jiff because `Etc/Unknown` isn't in the database, so 
`get()` has to handle it first. Here, names are looked up in a table built from 
`jiff_tzdb::available()` before `bundled_tz` is called, so `Etc/Unknown` is 
rejected as an invalid timezone and never reaches this function. Adding the 
check here wouldn't change that.
   
   Rejecting it is intentional for now, because this feature accepts the same 
names as chrono-tz, which doesn't have `Etc/Unknown`. `test_invalid_timezone` 
covers this. If we want to accept it once jiff becomes the default, it will 
need a table entry as well as the `TimeZone::unknown()` special case.
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to