You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by al...@apache.org on 2021/11/22 21:01:06 UTC
[arrow-rs] 01/01: Fix bug in temporal utilities due to DST being ignored. (#955)
This is an automated email from the ASF dual-hosted git repository.
alamb pushed a commit to branch cherry_pick_9703f989
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
commit c6518bbf36a15371ccb8abdd2c686a28e556fc00
Author: Navin <na...@novemberkilo.com>
AuthorDate: Sat Nov 20 23:04:58 2021 +1100
Fix bug in temporal utilities due to DST being ignored. (#955)
* Check behaviour of temporal utilities for DST.
* Fix temporal util bug ignoring dst.
* Refactor macro for efficiency.
---
arrow/src/compute/kernels/temporal.rs | 92 ++++++++++++++++++-----------------
1 file changed, 48 insertions(+), 44 deletions(-)
diff --git a/arrow/src/compute/kernels/temporal.rs b/arrow/src/compute/kernels/temporal.rs
index 269f5cb..317f8bb 100644
--- a/arrow/src/compute/kernels/temporal.rs
+++ b/arrow/src/compute/kernels/temporal.rs
@@ -47,23 +47,44 @@ macro_rules! extract_component_from_array {
"Expected format [+-]XX:XX".to_string()
)
} else {
- let fixed_offset = match parse(&mut $parsed, $tz, StrftimeItems::new("%z")) {
+ 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) => fo,
+ Ok(fo) => Some(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),
- },
+ _ => None,
};
+
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 fixed_offset_from_parsed {
+ Some(fo) => fo,
+ None => 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 +98,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<FixedOffset> {
- None
-}
-
-/// Parse the given string into a string representing fixed-offset
-#[cfg(feature = "chrono-tz")]
-pub fn using_chrono_tz(tz: &str) -> Option<FixedOffset> {
- use chrono::{Offset, TimeZone};
- tz.parse::<chrono_tz::Tz>()
- .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<FixedOffset> {
- 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
@@ -448,6 +452,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() {
@@ -462,22 +482,6 @@ mod tests {
#[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() {
let sydney_tz = "Australia/Sydney".to_string();
let sydney_offset_without_dst = FixedOffset::east(10 * 60 * 60);