From b340d2cc6ad34d2a5f3fc6cae48ccf7d1a05d780 Mon Sep 17 00:00:00 2001 From: Navin Keswani Date: Sun, 31 Oct 2021 17:53:56 +1100 Subject: [PATCH 1/3] Check behaviour of temporal utilities for DST. --- arrow/src/compute/kernels/temporal.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/arrow/src/compute/kernels/temporal.rs b/arrow/src/compute/kernels/temporal.rs index 269f5cbb735..f2d4fa2fe23 100644 --- a/arrow/src/compute/kernels/temporal.rs +++ b/arrow/src/compute/kernels/temporal.rs @@ -448,6 +448,22 @@ mod tests { assert_eq!(15, b.value(0)); } + #[cfg(feature = "chrono-tz")] + #[test] + fn test_temporal_array_timestamp_hour_with_dst_timezone_using_chrono_tz() { + // + // 1635577147 converts to 2021-10-30 17:59:07 in time zone Australia/Sydney (AEDT) + // The offset (difference to UTC) is +11:00. Note that daylight savings is in effect on 2021-10-30. + // When daylight savings is not in effect, Australia/Sydney has an offset difference of +10:00. + + let a = TimestampMillisecondArray::from_opt_vec( + vec![Some(1635577147000)], + Some("Australia/Sydney".to_string()), + ); + let b = hour(&a).unwrap(); + assert_eq!(17, b.value(0)); + } + #[cfg(not(feature = "chrono-tz"))] #[test] fn test_temporal_array_timestamp_hour_with_timezone_using_chrono_tz() { From 7e0e18775176dcc0fd8ae586d508ca42089ed733 Mon Sep 17 00:00:00 2001 From: Navin Keswani Date: Sun, 7 Nov 2021 18:30:15 +1100 Subject: [PATCH 2/3] Fix temporal util bug ignoring dst. --- arrow/src/compute/kernels/temporal.rs | 85 ++++++++++++--------------- 1 file changed, 37 insertions(+), 48 deletions(-) diff --git a/arrow/src/compute/kernels/temporal.rs b/arrow/src/compute/kernels/temporal.rs index f2d4fa2fe23..a70b599770a 100644 --- a/arrow/src/compute/kernels/temporal.rs +++ b/arrow/src/compute/kernels/temporal.rs @@ -47,23 +47,45 @@ macro_rules! extract_component_from_array { "Expected format [+-]XX:XX".to_string() ) } else { - let fixed_offset = match parse(&mut $parsed, $tz, StrftimeItems::new("%z")) { - Ok(_) => match $parsed.to_fixed_offset() { - Ok(fo) => fo, - err => return_compute_error_with!("Invalid timezone", err), - }, - _ => match using_chrono_tz($tz) { - Some(fo) => fo, - err => return_compute_error_with!("Unable to parse timezone", err), - }, - }; for i in 0..$array.len() { if $array.is_null(i) { $builder.append_null()?; } else { - match $array.$using(i, fixed_offset) { - Some(dt) => $builder.append_value(dt.$extract_fn() as i32)?, - None => $builder.append_null()?, + match $array.value_as_datetime(i) { + Some(utc) => { + let fixed_offset = match parse( + &mut $parsed, + $tz, + StrftimeItems::new("%z"), + ) { + Ok(_) => match $parsed.to_fixed_offset() { + Ok(fo) => fo, + err => return_compute_error_with!( + "Invalid timezone", + err + ), + }, + _ => match using_chrono_tz_and_utc_naive_date_time( + $tz, utc, + ) { + Some(fo) => fo, + err => return_compute_error_with!( + "Unable to parse timezone", + err + ), + }, + }; + match $array.$using(i, fixed_offset) { + Some(dt) => { + $builder.append_value(dt.$extract_fn() as i32)? + } + None => $builder.append_null()?, + } + } + err => return_compute_error_with!( + "Unable to read value as datetime", + err + ), } } } @@ -77,31 +99,14 @@ macro_rules! return_compute_error_with { }; } -/// Parse the given string into a string representing fixed-offset -#[cfg(not(feature = "chrono-tz"))] -pub fn using_chrono_tz(_: &str) -> Option { - None -} - -/// Parse the given string into a string representing fixed-offset -#[cfg(feature = "chrono-tz")] -pub fn using_chrono_tz(tz: &str) -> Option { - use chrono::{Offset, TimeZone}; - tz.parse::() - .map(|tz| { - tz.offset_from_utc_datetime(&chrono::NaiveDateTime::from_timestamp(0, 0)) - .fix() - }) - .ok() -} - #[cfg(not(feature = "chrono-tz"))] pub fn using_chrono_tz_and_utc_naive_date_time( _tz: &str, _utc: chrono::NaiveDateTime, ) -> Option { - Some(FixedOffset::east(0)) + None } + /// Parse the given string into a string representing fixed-offset that is correct as of the given /// UTC NaiveDateTime. /// Note that the offset is function of time and can vary depending on whether daylight savings is @@ -476,22 +481,6 @@ mod tests { assert!(matches!(hour(&a), Err(ArrowError::ComputeError(_)))) } - #[cfg(feature = "chrono-tz")] - #[test] - fn test_using_chrono_tz() { - let sydney_offset = FixedOffset::east(10 * 60 * 60); - assert_eq!( - using_chrono_tz(&"Australia/Sydney".to_string()), - Some(sydney_offset) - ); - - let nyc_offset = FixedOffset::west(5 * 60 * 60); - assert_eq!( - using_chrono_tz(&"America/New_York".to_string()), - Some(nyc_offset) - ); - } - #[cfg(feature = "chrono-tz")] #[test] fn test_using_chrono_tz_and_utc_naive_date_time() { From a7aaedad4243245c037e705b5c2a1440c9506c45 Mon Sep 17 00:00:00 2001 From: Navin Keswani Date: Fri, 19 Nov 2021 15:01:13 +1100 Subject: [PATCH 3/3] Refactor macro for efficiency. --- arrow/src/compute/kernels/temporal.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/arrow/src/compute/kernels/temporal.rs b/arrow/src/compute/kernels/temporal.rs index a70b599770a..317f8bb15e4 100644 --- a/arrow/src/compute/kernels/temporal.rs +++ b/arrow/src/compute/kernels/temporal.rs @@ -47,25 +47,24 @@ macro_rules! extract_component_from_array { "Expected format [+-]XX:XX".to_string() ) } else { + let tz_parse_result = parse(&mut $parsed, $tz, StrftimeItems::new("%z")); + let fixed_offset_from_parsed = match tz_parse_result { + Ok(_) => match $parsed.to_fixed_offset() { + Ok(fo) => Some(fo), + err => return_compute_error_with!("Invalid timezone", err), + }, + _ => None, + }; + for i in 0..$array.len() { if $array.is_null(i) { $builder.append_null()?; } else { match $array.value_as_datetime(i) { Some(utc) => { - let fixed_offset = match parse( - &mut $parsed, - $tz, - StrftimeItems::new("%z"), - ) { - Ok(_) => match $parsed.to_fixed_offset() { - Ok(fo) => fo, - err => return_compute_error_with!( - "Invalid timezone", - err - ), - }, - _ => match using_chrono_tz_and_utc_naive_date_time( + let fixed_offset = match fixed_offset_from_parsed { + Some(fo) => fo, + None => match using_chrono_tz_and_utc_naive_date_time( $tz, utc, ) { Some(fo) => fo,