mhilton commented on code in PR #22114: URL: https://github.com/apache/datafusion/pull/22114#discussion_r4291893859
########## datafusion/common/src/scalar/mod.rs: ########## @@ -109,6 +110,37 @@ const NANOS_PER_DAY: i64 = MICROS_PER_DAY * 1_000; const MICROS_PER_MILLISECOND: i64 = 1_000; const NANOS_PER_MILLISECOND: i64 = 1_000_000; +/// Returns the first instant of `date` in `tz` (UTC if `tz` is `None`) as a +/// UTC [`NaiveDateTime`]. +/// +/// This is local midnight unless the timezone skips midnight that day (for +/// example a daylight saving transition from 00:00 to 01:00), in which case +/// it is the first instant after the gap. If midnight occurs twice (a +/// transition back from 01:00 to 00:00) the earlier instant is returned. Review Comment: If you want a reference for this it is the convention described in [RFC5545ยง3.3.5](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.5) ########## datafusion/common/src/scalar/mod.rs: ########## @@ -109,6 +110,37 @@ const NANOS_PER_DAY: i64 = MICROS_PER_DAY * 1_000; const MICROS_PER_MILLISECOND: i64 = 1_000; const NANOS_PER_MILLISECOND: i64 = 1_000_000; +/// Returns the first instant of `date` in `tz` (UTC if `tz` is `None`) as a +/// UTC [`NaiveDateTime`]. +/// +/// This is local midnight unless the timezone skips midnight that day (for +/// example a daylight saving transition from 00:00 to 01:00), in which case +/// it is the first instant after the gap. If midnight occurs twice (a +/// transition back from 01:00 to 00:00) the earlier instant is returned. +fn first_instant_of_date(date: NaiveDate, tz: Option<&Tz>) -> Option<NaiveDateTime> { + let midnight = date.and_hms_opt(0, 0, 0)?; + let Some(tz) = tz else { + return Some(midnight); + }; + match tz.from_local_datetime(&midnight) { + LocalResult::Single(dt) => Some(dt.naive_utc()), + LocalResult::Ambiguous(earliest, _) => Some(earliest.naive_utc()), + LocalResult::None => { + // Midnight falls in a gap, so the first instant of the day is the + // instant of the transition itself. Find the offset in effect + // before the transition by probing a day earlier (any offset is + // less than a day, so that instant is before the transition), and + // use it to convert midnight to the transition instant. + let probe = midnight.checked_sub_signed(Duration::days(1))?; + let offset_before = tz.offset_from_utc_datetime(&probe).fix(); Review Comment: I'm pretty sure this works, but it's baffling.`midnight` is a local time so therefore is `probe`. I think there needs to be a comment saying why it is fine to use it as `UTC`, I think that's because we just care about some timestamp that is sufficiently before the discontinuity and it doesn't matter exactly what that time is. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
