Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix bug in temporal utilities due to DST being ignored. #955

Merged
merged 3 commits into from
Nov 20, 2021
Merged

Fix bug in temporal utilities due to DST being ignored. #955

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b340d2c
Check behaviour of temporal utilities for DST.
novemberkilo Oct 31, 2021
7e0e187
Fix temporal util bug ignoring dst.
novemberkilo Nov 7, 2021
a7aaeda
Refactor macro for efficiency.
novemberkilo Nov 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 48 additions & 44 deletions arrow/src/compute/kernels/temporal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
),
}
}
}
Expand All @@ -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
Expand Down Expand Up @@ -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() {
Expand All @@ -460,22 +480,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() {
Expand Down