adriangb commented on code in PR #25099:
URL: https://github.com/apache/datafusion/pull/25099#discussion_r3992932449
##########
.github/actions/setup-builder/action.yaml:
##########
@@ -29,7 +29,8 @@ runs:
shell: bash
run: |
RETRY=("ci/scripts/retry" timeout 120)
- "${RETRY[@]}" apt-get update
+ rm -f /etc/apt/sources.list.d/google-chrome.list
+ "${RETRY[@]}" apt-get update || true
Review Comment:
**Blocker.** This PR now changes six workflow files. They remove the Google
Chrome apt list and add `|| true` after `apt-get update`.
These changes are not part of the fix, and the PR description does not
mention them. `|| true` hides all apt failures, also the permanent ones. `main`
does not have this change.
Please remove the three `ci:` commits (838b442019, 340c693aa0, 72202c02c9).
Your branch is 328 commits behind `main`, and a rebase on `main` is the best
way to solve the CI problem:
```bash
git fetch upstream main
git rebase -i upstream/main # drop the three "ci:" commits
```
##########
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:**
+/// Conversions between timezone-naive and timezone-aware timestamps are
+/// mathematically bijective (shifting the physical value by the timezone
offset),
+/// rather than many-to-one lossy. However, we return `true` here to block
unwrapping
+/// as an intentionally conservative guard. If we returned `false`,
`unwrap_cast_in_comparison`
+/// would strip the cast but fail to shift the underlying literal, returning
incorrect
+/// query results. (A robust alternative would be to allow the unwrap and
shift the literal,
+/// preserving pushdown and pruning.) Only UTC-equivalent timezones (where the
shift is
+/// exactly zero) are allowed to bypass this guard.
Review Comment:
**Fix before merge.** The cast is not bijective. A naive local time in a DST
gap has no instant. A naive local time in a DST fold has two instants. On this
branch, both cases give an error:
```sql
SET datafusion.execution.time_zone = 'America/New_York';
SELECT TIMESTAMP '2024-11-03T01:30:00'::timestamptz; -- fold
SELECT TIMESTAMP '2024-03-10T02:30:00'::timestamptz; -- gap
-- Arrow error: Cast error: Cannot cast timezone to different timezone
```
This is the real reason that "shift the literal instead" is not a small
change. That alternative must handle these two cases first (see
https://github.com/apache/arrow-rs/pull/11038).
The conservative guard is the correct choice. But the comment must give the
correct reason for it. The suggestion below also describes the one-direction
change from my next comment.
```suggestion
/// **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.
```
##########
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:**
+/// Conversions between timezone-naive and timezone-aware timestamps are
+/// mathematically bijective (shifting the physical value by the timezone
offset),
+/// rather than many-to-one lossy. However, we return `true` here to block
unwrapping
+/// as an intentionally conservative guard. If we returned `false`,
`unwrap_cast_in_comparison`
+/// would strip the cast but fail to shift the underlying literal, returning
incorrect
+/// query results. (A robust alternative would be to allow the unwrap and
shift the literal,
+/// preserving pushdown and pruning.) Only UTC-equivalent timezones (where the
shift is
+/// exactly zero) are allowed to bypass this guard.
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(_, from_tz), DataType::Timestamp(_, to_tz)) =
+ (from_type, to_type)
+ {
+ match (from_tz, to_tz) {
+ (Some(tz), None) | (None, Some(tz))
+ if !is_zero_offset_timezone(tz.as_ref()) =>
+ {
+ return true;
+ }
+ _ => {}
+ }
+ }
Review Comment:
**Fix before merge.** Only one direction needs the guard. Arrow shifts the
value only for `Timestamp(None) -> Timestamp(Some(tz))`. The opposite cast is a
re-label of the same integer (see `_ => converted` in arrow-cast). This query
shows it:
```sql
SET datafusion.execution.time_zone = 'Asia/Singapore';
CREATE TABLE u AS SELECT '2024-10-31T16:00:00Z'::timestamptz AS tstz;
SELECT tstz::timestamp FROM u;
-- 2024-10-31T16:00:00 (the UTC wall clock, no shift)
```
Thus `CAST(tstz_col AS timestamp) = naive_literal` was unwrapped correctly
before this PR. With this PR the cast stays, and the rows are the same. I
checked this on this branch and on a build without the fix.
The `(None, Some(tz))` arm (a naive literal, a timezone-aware target) loses
this optimization and gains nothing. The suggestion keeps only the arm that the
bug needs. When the literal has a zone and the target does not, the two types
are timestamps, so the date checks after this block cannot return `true`, and
an early `return` is safe.
If you prefer to keep the wider guard, that is also fine with me. Then
please write in the comment that it is intentionally wider than necessary, and
keep the unit test as it is.
```suggestion
if let (DataType::Timestamp(_, Some(tz)), DataType::Timestamp(_, None)) =
(from_type, to_type)
{
return !is_zero_offset_timezone(tz.as_ref());
}
```
##########
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:**
+/// Conversions between timezone-naive and timezone-aware timestamps are
+/// mathematically bijective (shifting the physical value by the timezone
offset),
+/// rather than many-to-one lossy. However, we return `true` here to block
unwrapping
+/// as an intentionally conservative guard. If we returned `false`,
`unwrap_cast_in_comparison`
+/// would strip the cast but fail to shift the underlying literal, returning
incorrect
+/// query results. (A robust alternative would be to allow the unwrap and
shift the literal,
+/// preserving pushdown and pruning.) Only UTC-equivalent timezones (where the
shift is
+/// exactly zero) are allowed to bypass this guard.
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(_, from_tz), DataType::Timestamp(_, to_tz)) =
+ (from_type, to_type)
+ {
+ match (from_tz, to_tz) {
+ (Some(tz), None) | (None, Some(tz))
+ if !is_zero_offset_timezone(tz.as_ref()) =>
+ {
+ return true;
+ }
+ _ => {}
+ }
+ }
(is_date_type(from_type) && to_type.is_temporal())
|| (is_date_type(to_type) && from_type.is_temporal())
}
+/// Returns true if the timezone is known to have a fixed zero offset from UTC.
+///
+/// This is used to determine if a cast between a timezone-aware and
timezone-naive
+/// timestamp is lossy. If the timezone is strictly UTC-equivalent, the cast is
+/// a lossless re-labeling of the integer value.
+fn is_zero_offset_timezone(tz: &str) -> bool {
+ match tz {
+ // Standard UTC identifiers
+ "UTC" | "Etc/UTC" | "GMT" | "Etc/GMT" | "Greenwich" | "Z" => true,
+ // Common fixed offset zero strings parsed by Arrow
+ "+00:00" | "-00:00" | "+0:00" | "-0:00" => true,
+ _ => false,
+ }
+}
Review Comment:
**Fix before merge.** Some entries in this list are not valid Arrow
timezones. Arrow rejects `+0:00`, `-0:00` and `Z`:
```
Arrow error: Parser error: Invalid timezone "+0:00": failed to parse timezone
Arrow error: Parser error: Invalid timezone "Z": failed to parse timezone
```
Thus these entries never match. At the same time, Arrow accepts `+0000`,
`+00`, `-0000`, `Zulu`, `UCT`, `Universal` and `Etc/GMT0`, and this list does
not contain them. For all of them the cast stays. I checked this with `EXPLAIN`.
The direction of the error is safe. But the list is not correct today, and
this shows that a list is hard to keep correct.
The suggestion parses the three fixed-offset shapes that Arrow accepts, and
keeps a list only for the IANA aliases of UTC. Each name in the list is in
chrono-tz. This also solves the DST problem that you described: a fixed offset
does not change with the season, and the list has no geographic zones.
```suggestion
/// 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"00" | b"0000" | b"00:00")
),
}
}
```
##########
datafusion/expr-common/src/casts.rs:
##########
@@ -998,6 +1035,31 @@ 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()));
+
+ // Naive <-> UTC is NOT lossy (UTC offset is 0, so literal cast is
exact)
+ assert!(!is_lossy_temporal_cast(&ts_naive, &ts_utc));
+ assert!(!is_lossy_temporal_cast(&ts_utc, &ts_naive));
+ assert!(!is_lossy_temporal_cast(&ts_naive, &ts_etc_utc));
+ assert!(!is_lossy_temporal_cast(&ts_naive, &ts_gmt));
+
+ // Naive <-> Non-UTC is lossy because it ignores session timezone
+ assert!(is_lossy_temporal_cast(&ts_naive, &ts_sgt));
+ assert!(is_lossy_temporal_cast(&ts_sgt, &ts_naive));
Review Comment:
This test must change with the two suggestions above. It now covers two
fixed-offset spellings, and it shows that only one direction is lossy.
```suggestion
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));
```
##########
datafusion/sqllogictest/test_files/datetime/timestamps.slt:
##########
@@ -5529,3 +5533,75 @@ query P
SELECT date_bin(NULL, TIMESTAMP '2023-01-01 12:30:00', TIMESTAMP '2023-01-01
12:00:00')
----
NULL
+
+# Issue 25095: Optimizer incorrectly unwrapping timestamp cast when session
timezone is not UTC
+statement ok
+set datafusion.execution.time_zone = 'Asia/Singapore';
+
+statement ok
+create table t_25095 as select TIMESTAMP '2024-11-01T00:00:00' as ts;
+
+statement ok
+create table u_25095 as select '2024-10-31T16:00:00Z'::timestamptz as tstz;
+
+# 2024-11-01 00:00 in Singapore is 2024-10-31 16:00 UTC
+query I
+select count(*) from t_25095 where ts::timestamptz =
'2024-10-31T16:00:00Z'::timestamptz;
+----
+1
+
+query I
+select count(*) from t_25095 where ts::timestamptz =
'2024-11-01T00:00:00Z'::timestamptz;
+----
+0
+
+# the same rewrite occurs for an implicit coercion
+query I
+select count(*) from t_25095 where ts = '2024-10-31T16:00:00Z'::timestamptz;
+----
+1
+
+# control: a column against a column, thus the optimizer unwraps nothing
+query I
+select count(*) from t_25095, u_25095 where t_25095.ts::timestamptz =
u_25095.tstz;
+----
+1
+
+# A timezone-aware column against a timezone-naive literal
Review Comment:
**Nit.** This test does not exercise the guard. The coercion casts the
literal, not the column, and that cast folds to a constant. `EXPLAIN` shows
`tstz = 1730390400000000000` on this branch and on `main`.
The test is still a good test. But please change the comment, so that a
reader does not think it tests the other direction of the guard.
```suggestion
# A timezone-aware column against a timezone-naive literal. The coercion
casts the
# literal, not the column, so this query does not go through unwrap_cast at
all
```
##########
datafusion/sqllogictest/test_files/datetime/timestamps.slt:
##########
@@ -5529,3 +5533,75 @@ query P
SELECT date_bin(NULL, TIMESTAMP '2023-01-01 12:30:00', TIMESTAMP '2023-01-01
12:00:00')
----
NULL
+
+# Issue 25095: Optimizer incorrectly unwrapping timestamp cast when session
timezone is not UTC
+statement ok
+set datafusion.execution.time_zone = 'Asia/Singapore';
+
+statement ok
+create table t_25095 as select TIMESTAMP '2024-11-01T00:00:00' as ts;
+
+statement ok
+create table u_25095 as select '2024-10-31T16:00:00Z'::timestamptz as tstz;
+
+# 2024-11-01 00:00 in Singapore is 2024-10-31 16:00 UTC
+query I
+select count(*) from t_25095 where ts::timestamptz =
'2024-10-31T16:00:00Z'::timestamptz;
+----
+1
+
+query I
+select count(*) from t_25095 where ts::timestamptz =
'2024-11-01T00:00:00Z'::timestamptz;
+----
+0
+
+# the same rewrite occurs for an implicit coercion
+query I
+select count(*) from t_25095 where ts = '2024-10-31T16:00:00Z'::timestamptz;
+----
+1
+
+# control: a column against a column, thus the optimizer unwraps nothing
+query I
+select count(*) from t_25095, u_25095 where t_25095.ts::timestamptz =
u_25095.tstz;
+----
+1
+
+# A timezone-aware column against a timezone-naive literal
+query I
+select count(*) from u_25095 where tstz = TIMESTAMP '2024-11-01T00:00:00';
+----
+1
+
+# Set session timezone back to UTC and demonstrate that the cast IS unwrapped
+statement ok
+set datafusion.execution.time_zone = 'UTC';
+
+# The explain output should show that the cast s::timestamptz has been
removed
Review Comment:
**Nit.** There is a tab character in this comment.
```suggestion
# The explain output should show that the cast `ts::timestamptz` has been
removed
```
--
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]
