Ruchirtripathi commented on code in PR #25099:
URL: https://github.com/apache/datafusion/pull/25099#discussion_r3971501073
##########
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"
+ {
Review Comment:
Thank you for the feedback! I went with your second suggestion. I moved the
hardcoded list into a new `is_zero_offset_timezone` function and added comments
explaining why each timezone is included.
We can’t simply parse the timezone using `arrow::array::timezone::Tz` and
check its offset because the offset can change depending on the time of year
due to Daylight Saving Time. For example, `Europe/London` has a UTC offset of
zero during winter. If we checked the offset dynamically during query planning,
it could incorrectly be treated as a zero-offset timezone and cause bugs.
Because of this, keeping a strict whitelist of timezones that are
permanently equivalent to UTC is the safest approach.
--
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]
