adriangb commented on code in PR #25099:
URL: https://github.com/apache/datafusion/pull/25099#discussion_r4135105365
##########
datafusion/expr-common/src/casts.rs:
##########
@@ -113,17 +113,54 @@ fn is_date_type(data_type: &DataType) -> bool {
/// `Date64` carrying sub-day milliseconds would lose them. This is not a
licence to
/// drop them - [`try_cast_numeric_literal`] returns `None` for a `Date64`
value not
/// divisible by 86_400_000, so an inexact `Date64` -> `Date32` fold never
happens.
+///
+/// **Timezone shifts:** Arrow casts a naive timestamp to a timezone-aware one
by
+/// interpreting the naive value as local time in the target zone and shifting
it by
+/// that zone's offset. `try_cast_numeric_literal` cannot apply the shift: it
re-labels
+/// the integer. So a timezone-aware literal is reported as lossy against a
naive
+/// target unless the zone's offset is always zero. The cast is not a
bijection either:
+/// a local time in a DST gap has no instant, and a local time in a DST fold
has two,
+/// so "shift the literal instead" is not a drop-in alternative.
+///
+/// The opposite cast (timezone-aware -> naive) is a plain re-label in Arrow,
so a naive
+/// literal is never lossy against a timezone-aware target.
fn is_lossy_temporal_cast(from_type: &DataType, to_type: &DataType) -> bool {
if from_type == to_type {
return false;
}
if is_date_type(from_type) && is_date_type(to_type) {
return false;
}
+ if let (DataType::Timestamp(_, Some(tz)), DataType::Timestamp(_, None)) =
+ (from_type, to_type)
+ {
+ return !is_zero_offset_timezone(tz.as_ref());
+ }
(is_date_type(from_type) && to_type.is_temporal())
|| (is_date_type(to_type) && from_type.is_temporal())
}
+/// Returns true if `tz` is a timezone whose offset from UTC is always zero,
so that
+/// casting a naive timestamp to `Timestamp(_, Some(tz))` does not move the
value.
+///
+/// Arrow's timezone parser accepts three fixed-offset shapes (`+HH:MM`,
`+HHMM`,
+/// `+HH`, with either sign) and otherwise an IANA name. A fixed offset is
zero when
+/// all of its digits are zero. IANA names are accepted only from the list of
UTC
+/// aliases below: a geographic zone such as `Europe/London` has a zero offset
for
+/// part of the year only, so it is never accepted. The IANA lookup is
case-sensitive,
+/// so `utc` is not a valid timezone and does not need to be listed.
+fn is_zero_offset_timezone(tz: &str) -> bool {
+ match tz {
+ "UTC" | "Etc/UTC" | "UCT" | "Etc/UCT" | "Universal" | "Etc/Universal"
Review Comment:
Getting this list wrong is asymmetric: a missing alias only loses the
unwrap, but an extra zone that is UTC only some of the time (`Europe/London`,
`Antarctica/Troll`) gives wrong results. I've checked these and they look
correct: of the 597 zones in `chrono-tz` 0.10.4 (the version in `Cargo.lock`),
exactly these 18 have a zero offset at `NaiveDateTime::MIN`/`MAX` and every 6
hours from 1800 to 2200.
##########
datafusion/expr-common/src/casts.rs:
##########
@@ -998,6 +1035,42 @@ mod tests {
assert!(is_lossy_temporal_cast(&ts, &DataType::Date32));
}
+ #[test]
+ fn test_is_lossy_temporal_cast_timestamp_tz() {
+ let ts_naive = DataType::Timestamp(TimeUnit::Millisecond, None);
+ let ts_utc = DataType::Timestamp(TimeUnit::Millisecond,
Some("UTC".into()));
+ let ts_etc_utc =
+ DataType::Timestamp(TimeUnit::Millisecond, Some("Etc/UTC".into()));
+ let ts_gmt = DataType::Timestamp(TimeUnit::Millisecond,
Some("GMT".into()));
+ let ts_sgt =
+ DataType::Timestamp(TimeUnit::Millisecond,
Some("Asia/Singapore".into()));
+
+ let ts_zero_offset =
+ DataType::Timestamp(TimeUnit::Millisecond, Some("+00:00".into()));
+ let ts_zero_offset_short =
+ DataType::Timestamp(TimeUnit::Millisecond, Some("-0000".into()));
+ let ts_offset = DataType::Timestamp(TimeUnit::Millisecond,
Some("+08:00".into()));
+ // Zero-offset zone <-> naive is NOT lossy: the cast does not move the
value
+ assert!(!is_lossy_temporal_cast(&ts_naive, &ts_utc));
+ assert!(!is_lossy_temporal_cast(&ts_utc, &ts_naive));
+ assert!(!is_lossy_temporal_cast(&ts_etc_utc, &ts_naive));
+ assert!(!is_lossy_temporal_cast(&ts_gmt, &ts_naive));
+ assert!(!is_lossy_temporal_cast(&ts_zero_offset, &ts_naive));
+ assert!(!is_lossy_temporal_cast(&ts_zero_offset_short, &ts_naive));
+ // A non-zero zone literal against a naive target is lossy: Arrow
shifts the
+ // column by the zone offset, and the re-labeled literal would not be
shifted
+ assert!(is_lossy_temporal_cast(&ts_sgt, &ts_naive));
+ assert!(is_lossy_temporal_cast(&ts_offset, &ts_naive));
+ // A naive literal against a timezone-aware target is not lossy: Arrow
casts
+ // timezone-aware -> naive by re-labeling the value
+ assert!(!is_lossy_temporal_cast(&ts_naive, &ts_sgt));
+ assert!(!is_lossy_temporal_cast(&ts_naive, &ts_offset));
+
+ // Tz-aware <-> Tz-aware is not lossy (both are UTC under the hood)
Review Comment:
These assertions encode an assumption about Arrow rather than about our
code: naive -> tz-aware shifts by the zone offset, while tz-aware -> naive
re-labels without shifting. If an arrow-rs upgrade changed that, results would
be silently wrong with no change on our side. Could we add a test that runs
`cast_with_options` on a real value, e.g. 2024-07-01T12:00 naive ->
`Europe/London` gives -1h, `Europe/London` -> naive is unchanged, and naive ->
each UTC alias is unchanged?
For example:
```rust
#[test]
fn test_arrow_timezone_cast_semantics() {
use arrow::array::{AsArray, TimestampMillisecondArray};
use arrow::datatypes::TimestampMillisecondType;
// 2024-07-01T12:00:00, when Europe/London is on BST (+01:00)
let value = 1_719_835_200_000_i64;
let cast = |from: Option<&str>, to: Option<&str>| {
let array =
TimestampMillisecondArray::from(vec![value]).with_timezone_opt(from);
let to = DataType::Timestamp(TimeUnit::Millisecond,
to.map(Into::into));
let array = cast_with_options(&array, &to,
&CastOptions::default()).unwrap();
array.as_primitive::<TimestampMillisecondType>().value(0)
};
// naive -> tz-aware shifts by the zone offset
assert_eq!(cast(None, Some("Europe/London")), value - 3_600_000);
// tz-aware -> naive re-labels the value without shifting it
assert_eq!(cast(Some("Europe/London"), None), value);
// naive -> a zero-offset zone does not move the value
for tz in ["UTC", "Etc/GMT", "Zulu", "+00:00", "-0000"] {
assert_eq!(cast(None, Some(tz)), value, "{tz}");
}
}
```
##########
datafusion/expr-common/src/casts.rs:
##########
@@ -113,17 +113,54 @@ fn is_date_type(data_type: &DataType) -> bool {
/// `Date64` carrying sub-day milliseconds would lose them. This is not a
licence to
/// drop them - [`try_cast_numeric_literal`] returns `None` for a `Date64`
value not
/// divisible by 86_400_000, so an inexact `Date64` -> `Date32` fold never
happens.
+///
+/// **Timezone shifts:** Arrow casts a naive timestamp to a timezone-aware one
by
+/// interpreting the naive value as local time in the target zone and shifting
it by
+/// that zone's offset. `try_cast_numeric_literal` cannot apply the shift: it
re-labels
+/// the integer. So a timezone-aware literal is reported as lossy against a
naive
+/// target unless the zone's offset is always zero. The cast is not a
bijection either:
+/// a local time in a DST gap has no instant, and a local time in a DST fold
has two,
+/// so "shift the literal instead" is not a drop-in alternative.
+///
+/// The opposite cast (timezone-aware -> naive) is a plain re-label in Arrow,
so a naive
+/// literal is never lossy against a timezone-aware target.
fn is_lossy_temporal_cast(from_type: &DataType, to_type: &DataType) -> bool {
if from_type == to_type {
return false;
}
if is_date_type(from_type) && is_date_type(to_type) {
return false;
}
+ if let (DataType::Timestamp(_, Some(tz)), DataType::Timestamp(_, None)) =
+ (from_type, to_type)
+ {
+ return !is_zero_offset_timezone(tz.as_ref());
+ }
(is_date_type(from_type) && to_type.is_temporal())
|| (is_date_type(to_type) && from_type.is_temporal())
}
+/// Returns true if `tz` is a timezone whose offset from UTC is always zero,
so that
+/// casting a naive timestamp to `Timestamp(_, Some(tz))` does not move the
value.
+///
+/// Arrow's timezone parser accepts three fixed-offset shapes (`+HH:MM`,
`+HHMM`,
+/// `+HH`, with either sign) and otherwise an IANA name. A fixed offset is
zero when
+/// all of its digits are zero. IANA names are accepted only from the list of
UTC
+/// aliases below: a geographic zone such as `Europe/London` has a zero offset
for
+/// part of the year only, so it is never accepted. The IANA lookup is
case-sensitive,
+/// so `utc` is not a valid timezone and does not need to be listed.
+fn is_zero_offset_timezone(tz: &str) -> bool {
+ match tz {
+ "UTC" | "Etc/UTC" | "UCT" | "Etc/UCT" | "Universal" | "Etc/Universal"
+ | "Zulu" | "Etc/Zulu" | "GMT" | "Etc/GMT" | "GMT0" | "Etc/GMT0" |
"GMT+0"
+ | "Etc/GMT+0" | "GMT-0" | "Etc/GMT-0" | "Greenwich" | "Etc/Greenwich"
=> true,
+ _ => matches!(
+ tz.strip_prefix(['+', '-']).map(str::as_bytes),
+ Some(b"0" | b"00" | b"0000" | b"0:00" | b"00:00")
Review Comment:
`"+0"`, `"-0"` and `"+0:00"` can't reach this: Arrow's timezone parser only
accepts `+HH`, `+HHMM` and `+HH:MM`, and rejects those three with `Invalid
timezone`. Rather than hand-matching digit patterns, could we parse with
`arrow::array::timezone::Tz` (the parser the cast kernel uses) and check the
offset is zero? Then we accept exactly what Arrow accepts.
Something like this, which needs no new dependency:
```rust
use arrow::array::timezone::Tz;
fn is_zero_offset_timezone(tz: &str) -> bool {
match tz {
"UTC" | "Etc/UTC" | "UCT" | "Etc/UCT" | "Universal" | "Etc/Universal"
| "Zulu" | "Etc/Zulu" | "GMT" | "Etc/GMT" | "GMT0" | "Etc/GMT0" |
"GMT+0"
| "Etc/GMT+0" | "GMT-0" | "Etc/GMT-0" | "Greenwich" |
"Etc/Greenwich" => true,
// A fixed offset Arrow accepts (`+HH`, `+HHMM`, `+HH:MM`) whose
digits are all zero
_ => {
tz.starts_with(['+', '-'])
&& tz.parse::<Tz>().is_ok()
&& tz[1..].bytes().all(|b| matches!(b, b'0' | b':'))
}
}
}
```
--
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]
