mhilton commented on code in PR #9068:
URL: https://github.com/apache/arrow-datafusion/pull/9068#discussion_r1471076663
##########
datafusion/physical-expr/src/datetime_expressions.rs:
##########
@@ -564,8 +564,34 @@ fn _date_trunc_coarse_with_tz(
granularity: &str,
value: Option<DateTime<Tz>>,
) -> Result<Option<i64>> {
- let value = _date_trunc_coarse::<DateTime<Tz>>(granularity, value)?;
- Ok(value.and_then(|value| value.timestamp_nanos_opt()))
+ if let Some(value) = value {
+ let timezone = value.timezone();
+ let offset = value.offset().fix();
+ let local = value.naive_local();
+ let value = _date_trunc_coarse::<NaiveDateTime>(granularity,
Some(local))?;
+ let value = value.map(|value| match value.and_local_timezone(timezone)
{
+ LocalResult::None => {
+ // It is impossible to truncate from a time that does exist
into one that doesn't.
+ panic!("date_trunc produced impossible time")
+ }
+ LocalResult::Single(datetime) => datetime,
+ LocalResult::Ambiguous(datetime1, datetime2) => {
+ // Because we are truncating from an equally or more specific
time
+ // the original time must have been within the ambiguous local
time
+ // period. Therefore the offset of one of these times should
match the
+ // offset of the original time.
+ if datetime1.offset().fix() == offset {
+ datetime1
+ } else {
+ datetime2
+ }
+ }
+ });
+ Ok(value.and_then(|value| value.timestamp_nanos_opt()))
Review Comment:
I think this misses that the truncated time might be unambiguously in a
different offset from the original time. I think the offset is needed as a tie
breaker when the wall-clock time of the result could describe two times in the
local time zone, otherwise we ignore it.
--
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]
