adriangb commented on code in PR #11054:
URL: https://github.com/apache/arrow-rs/pull/11054#discussion_r4307479471
##########
arrow-cast/src/parse.rs:
##########
@@ -186,9 +203,7 @@ pub fn string_to_datetime<T: TimeZone>(timezone: &T, s:
&str) -> Result<DateTime
let date = parser.date().ok_or_else(|| err("error parsing date"))?;
if bytes.len() == 10 {
let datetime = date.and_time(NaiveTime::MIN);
- return timezone
- .from_local_datetime(&datetime)
- .single()
+ return resolve_local_datetime(timezone, &datetime)
Review Comment:
**Behaviour change (1/3): date-only input.** This was
`timezone.from_local_datetime(&datetime).single()`, which gave `None` (so an
error) when local midnight is ambiguous or skipped, for example
`America/Sao_Paulo` on 2018-11-04 or `America/Havana` on 2024-11-03. Now it
resolves to the later instant, or shifts forward past the gap.
##########
arrow-cast/src/local_time.rs:
##########
@@ -0,0 +1,100 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Resolving wall clock ("local") readings to instants.
+//!
+//! Reading a [`NaiveDateTime`] as a wall clock time in an IANA timezone does
not
+//! always identify a unique instant, because of daylight savings transitions.
+//! Everything that has to make that choice -- the cast kernel and the string
+//! parser -- goes through [`resolve_local_offset`] so that the two cannot
drift
+//! apart.
+
+use chrono::{DateTime, LocalResult, NaiveDateTime, Offset, TimeDelta,
TimeZone};
+
+/// Returns the offset to use when interpreting `local` as a wall clock reading
+/// in `tz`, or `None` if it cannot be resolved.
+///
+/// `None` is not expected in practice. With the current timezone database no
+/// reading reaches it, because every ambiguous or nonexistent reading resolves
+/// as described below. The `None` path is a safeguard against a future
+/// timezone database that breaks the assumptions of the gap handling. Callers
+/// then apply their usual error or null handling.
+///
+/// In an IANA timezone a wall clock reading does not always identify a unique
+/// instant, and this function picks one following the same rules as PostgreSQL
+/// and DuckDB:
+///
+/// * **Ambiguous** -- when the clocks go back ("fall back") the same reading
+/// occurs twice. The *later* instant is chosen, i.e. the offset in effect
+/// after the transition. For example `2024-11-03T01:30:00` in
+/// `America/New_York` is read as `-05:00` (EST), not `-04:00` (EDT).
+/// * **Nonexistent** -- when the clocks go forward ("spring forward") the
+/// reading never occurs. It is shifted forward by the length of the gap,
+/// which is the same as reading it with the offset in effect *before* the
+/// transition. For example `2024-03-10T02:30:00` in `America/New_York` is
+/// read as `-05:00` (EST) and therefore denotes `2024-03-10T03:30:00-04:00`.
+///
+/// Timezones with a fixed offset are never ambiguous and have no gaps.
+///
+/// See <https://github.com/apache/arrow-rs/issues/11037> for the PostgreSQL
and
+/// ICU (DuckDB) sources these rules are taken from.
+pub(crate) fn resolve_local_offset<T: TimeZone>(
+ tz: &T,
+ local: &NaiveDateTime,
+) -> Option<T::Offset> {
+ match tz.offset_from_local_datetime(local) {
+ LocalResult::Single(offset) => Some(offset),
+ LocalResult::Ambiguous(_earlier, later) => Some(later),
+ LocalResult::None => {
+ // The reading falls in a gap. Recover the offset in effect before
+ // the transition by probing 24 hours earlier.
+ //
+ // Two separate properties of the timezone database make this
sound:
+ //
+ // 1. No local gap is longer than 24 hours, so the probe lands
+ // outside this gap and is itself resolvable. Seven zones have a
+ // gap of exactly 24 hours -- the dateline changes, such as
+ // `Pacific/Apia` in 2011 and `Pacific/Kiritimati` in 1994. At
the
+ // last second of one of those the probe lands one second before
+ // the gap starts, so the true margin here is one second, not a
+ // comfortable one.
+ // 2. No two transitions are closer together than 24 hours, so the
+ // offset the probe finds is the one in effect immediately
before
+ // this transition, and not some older offset. The smallest
+ // observed interval is 167 hours (`America/Boa_Vista`, 2000).
+ //
+ // Property 1 is what makes the probe resolvable; property 2 is
what
+ // makes the answer correct. If the probe is still unresolvable,
give
+ // up and let the caller apply the usual error / null handling.
+ tz.offset_from_local_datetime(&(*local - TimeDelta::hours(24)))
+ .earliest()
+ }
+ }
+}
+
+/// Reads `local` as a wall clock time in `tz`, resolving daylight savings
+/// ambiguities and gaps as described on [`resolve_local_offset`].
+///
+/// This is [`TimeZone::from_local_datetime`] with the two unresolvable
+/// [`LocalResult`] cases decided rather than rejected.
+pub(crate) fn resolve_local_datetime<T: TimeZone>(
Review Comment:
New in the fix commit. It only wraps `resolve_local_offset` (moved unchanged
in the first commit) to return a `DateTime<T>`, which is the type the three
parser call sites need.
##########
arrow-cast/src/parse.rs:
##########
@@ -219,9 +232,7 @@ pub fn string_to_datetime<T: TimeZone>(timezone: &T, s:
&str) -> Result<DateTime
// Parse remainder of string as timezone
let parsed_tz: Tz = s[tz_offset..].trim_start().parse()?;
- let parsed = parsed_tz
- .from_local_datetime(&datetime)
- .single()
+ let parsed = resolve_local_datetime(&parsed_tz, &datetime)
Review Comment:
**Behaviour change (3/3): datetime with a trailing IANA zone name**, for
example `2024-11-03 01:30:00 America/New_York`. This was `.single()` before. It
now uses the same policy as the other two call sites, which is what PostgreSQL
does.
##########
arrow-cast/src/parse.rs:
##########
@@ -207,9 +222,7 @@ pub fn string_to_datetime<T: TimeZone>(timezone: &T, s:
&str) -> Result<DateTime
}
if bytes.len() <= tz_offset {
- return timezone
- .from_local_datetime(&datetime)
- .single()
+ return resolve_local_datetime(timezone, &datetime)
Review Comment:
**Behaviour change (2/3): naive datetime read in the target timezone.** This
is the `Utf8 -> Timestamp(_, Some(tz))` cast path. It was `.single()` before,
so for example `2024-03-10 02:30:00` in `America/New_York` became an error, or
NULL with `safe: true`. Now it gives `03:30 EDT`.
##########
arrow-cast/src/cast/temporal.rs:
##########
@@ -404,73 +406,14 @@ pub(crate) fn timestamp_to_date64<T: ArrowTimestampType>(
Ok(Arc::new(array))
}
-/// Returns the offset to use when interpreting `local` as a wall clock reading
-/// in `tz`, or `None` if it cannot be resolved.
-///
-/// `None` is not expected in practice. With the current timezone database no
-/// reading reaches it, because every ambiguous or nonexistent reading resolves
-/// as described below. The `None` path is a safeguard against a future
-/// timezone database that breaks the assumptions of the gap handling. Callers
-/// then apply their usual error or null handling.
-///
-/// In an IANA timezone a wall clock reading does not always identify a unique
-/// instant, and this function picks one following the same rules as PostgreSQL
-/// and DuckDB:
-///
-/// * **Ambiguous** -- when the clocks go back ("fall back") the same reading
-/// occurs twice. The *later* instant is chosen, i.e. the offset in effect
-/// after the transition. For example `2024-11-03T01:30:00` in
-/// `America/New_York` is read as `-05:00` (EST), not `-04:00` (EDT).
-/// * **Nonexistent** -- when the clocks go forward ("spring forward") the
-/// reading never occurs. It is shifted forward by the length of the gap,
-/// which is the same as reading it with the offset in effect *before* the
-/// transition. For example `2024-03-10T02:30:00` in `America/New_York` is
-/// read as `-05:00` (EST) and therefore denotes `2024-03-10T03:30:00-04:00`.
-///
-/// Timezones with a fixed offset are never ambiguous and have no gaps.
-///
-/// See <https://github.com/apache/arrow-rs/issues/11037> for the PostgreSQL
and
-/// ICU (DuckDB) sources these rules are taken from.
-fn resolve_local_offset(tz: &Tz, local: &NaiveDateTime) -> Option<FixedOffset>
{
- match tz.offset_from_local_datetime(local) {
- LocalResult::Single(offset) => Some(offset.fix()),
- LocalResult::Ambiguous(_earlier, later) => Some(later.fix()),
- LocalResult::None => {
- // The reading falls in a gap. Recover the offset in effect before
- // the transition by probing 24 hours earlier.
- //
- // Two separate properties of the timezone database make this
sound:
- //
- // 1. No local gap is longer than 24 hours, so the probe lands
- // outside this gap and is itself resolvable. Seven zones have a
- // gap of exactly 24 hours -- the dateline changes, such as
- // `Pacific/Apia` in 2011 and `Pacific/Kiritimati` in 1994. At
the
- // last second of one of those the probe lands one second before
- // the gap starts, so the true margin here is one second, not a
- // comfortable one.
- // 2. No two transitions are closer together than 24 hours, so the
- // offset the probe finds is the one in effect immediately
before
- // this transition, and not some older offset. The smallest
- // observed interval is 167 hours (`America/Boa_Vista`, 2000).
- //
- // Property 1 is what makes the probe resolvable; property 2 is
what
- // makes the answer correct. If the probe is still unresolvable,
give
- // up and let the caller apply the usual error / null handling.
- tz.offset_from_local_datetime(&(*local - TimeDelta::hours(24)))
- .earliest()
- .map(|offset| offset.fix())
- }
- }
-}
-
pub(crate) fn adjust_timestamp_to_timezone<T: ArrowTimestampType>(
array: PrimitiveArray<Int64Type>,
to_tz: &Tz,
cast_options: &CastOptions,
) -> Result<PrimitiveArray<Int64Type>, ArrowError> {
let adjust = |o| {
let local = as_datetime::<T>(o)?;
- let offset = resolve_local_offset(to_tz, &local)?;
+ let offset = resolve_local_offset(to_tz, &local)?.fix();
Review Comment:
No behaviour change. `resolve_local_offset` now returns `T::Offset` because
it is generic over the timezone type, so this caller calls `.fix()` itself.
Before, the function called `.fix()` internally.
--
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]
