Ruchirtripathi commented on code in PR #25099:
URL: https://github.com/apache/datafusion/pull/25099#discussion_r4103409618
##########
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:
Great catch! I completely agree that we shouldn't lose the optimization for
the timezone-aware -> naive cast since
it's just a re-label. I've updated the logic to only apply the guard to the
direction that actually needs it, just as you suggested.
--
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]
