Copilot commented on code in PR #25099:
URL: https://github.com/apache/datafusion/pull/25099#discussion_r3968630592
##########
datafusion/sqllogictest/test_files/datetime/timestamps.slt:
##########
@@ -4301,7 +4301,6 @@ SELECT column1 FROM t_utc WHERE column1 <
'2024-02-01T00:00:00' AT TIME ZONE 'Am
query P
SELECT column1 FROM t_europe WHERE column1 = '2024-01-31T16:00:01' AT TIME
ZONE 'America/Los_Angeles';
----
-2024-02-01T00:00:01+01:00
Review Comment:
The PR description says the sqllogictest was previously asserting an
incorrect *empty* result and is being updated to reflect the correct behavior.
This diff removes the only expected row, which makes the expected result empty.
Please reconcile this mismatch: either the description is inverted for this
specific test, or this expectation change is going the wrong direction.
Concretely, re-run this query under a non-UTC session timezone and update
either the test expectation or the PR description to match the observed/correct
behavior.
##########
datafusion/expr-common/src/casts.rs:
##########
@@ -120,6 +120,21 @@ fn is_lossy_temporal_cast(from_type: &DataType, to_type:
&DataType) -> bool {
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)
+ && from_tz.is_some() != to_tz.is_some()
+ {
+ let tz = from_tz.as_ref().or(to_tz.as_ref()).unwrap().as_ref();
+ if tz != "UTC"
+ && tz != "+00:00"
+ && tz != "-00:00"
+ && tz != "+0:00"
+ && tz != "-0:00"
+ && tz != "Z"
+ {
+ return true;
+ }
+ }
Review Comment:
The UTC detection here is based on a hard-coded list of string spellings.
This is brittle (case variations like \"utc\", other canonical spellings like
\"Etc/UTC\"/\"GMT\", or alternative zero-offset formats may appear depending on
upstream parsing), and it risks either disabling optimizations unnecessarily or
(worse) misclassifying a zero-offset zone as non-UTC. A more robust approach
would be to normalize and/or parse the timezone and check whether it represents
a fixed zero offset (or is equivalent to UTC) using a single helper, rather
than enumerating string literals in-line.
##########
datafusion/expr-common/src/casts.rs:
##########
@@ -998,6 +1013,26 @@ 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_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));
+
+ // 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));
+
+ // Tz-aware <-> Tz-aware is not lossy (both are UTC under the hood)
+ assert!(!is_lossy_temporal_cast(&ts_utc, &ts_sgt));
+ assert!(!is_lossy_temporal_cast(&ts_sgt, &ts_utc));
+ }
Review Comment:
This test only exercises the \"UTC\" spelling for the zero-offset case, but
the implementation also special-cases \"+00:00\", \"Z\", and a few other
strings. It would be good to add assertions covering at least one or two of
those additional accepted UTC representations (and any expected
case-normalization behavior, if applicable) to prevent regressions where the
string matching changes.
--
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]
