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.

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

temporal conversion functions should work on negative input properly #2326

Merged
merged 4 commits into from Aug 5, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
123 changes: 114 additions & 9 deletions arrow/src/temporal_conversions.rs
Expand Up @@ -42,11 +42,13 @@ pub fn date32_to_datetime(v: i32) -> NaiveDateTime {
/// converts a `i64` representing a `date64` to [`NaiveDateTime`]
#[inline]
pub fn date64_to_datetime(v: i64) -> NaiveDateTime {
let (sec, milli_sec) = split_second(v, MILLISECONDS);

NaiveDateTime::from_timestamp(
// extract seconds from milliseconds
v / MILLISECONDS,
sec,
// discard extracted seconds and convert milliseconds to nanoseconds
(v % MILLISECONDS * MICROSECONDS) as u32,
milli_sec * MICROSECONDS as u32,
)
}

Expand Down Expand Up @@ -101,36 +103,59 @@ pub fn timestamp_s_to_datetime(v: i64) -> NaiveDateTime {
/// converts a `i64` representing a `timestamp(ms)` to [`NaiveDateTime`]
#[inline]
pub fn timestamp_ms_to_datetime(v: i64) -> NaiveDateTime {
let (sec, milli_sec) = split_second(v, MILLISECONDS);

NaiveDateTime::from_timestamp(
// extract seconds from milliseconds
v / MILLISECONDS,
sec,
// discard extracted seconds and convert milliseconds to nanoseconds
(v % MILLISECONDS * MICROSECONDS) as u32,
milli_sec * MICROSECONDS as u32,
)
}

/// converts a `i64` representing a `timestamp(us)` to [`NaiveDateTime`]
#[inline]
pub fn timestamp_us_to_datetime(v: i64) -> NaiveDateTime {
let (sec, micro_sec) = split_second(v, MICROSECONDS);

NaiveDateTime::from_timestamp(
// extract seconds from microseconds
v / MICROSECONDS,
sec,
// discard extracted seconds and convert microseconds to nanoseconds
(v % MICROSECONDS * MILLISECONDS) as u32,
micro_sec * MILLISECONDS as u32,
)
}

/// converts a `i64` representing a `timestamp(ns)` to [`NaiveDateTime`]
#[inline]
pub fn timestamp_ns_to_datetime(v: i64) -> NaiveDateTime {
let (sec, nano_sec) = split_second(v, NANOSECONDS);

NaiveDateTime::from_timestamp(
// extract seconds from nanoseconds
v / NANOSECONDS,
// discard extracted seconds
if v > 0 { (v % NANOSECONDS) as u32 } else { 0 },
sec, // discard extracted seconds
nano_sec,
)
}

///
#[inline]
pub(crate) fn split_second(v: i64, base: i64) -> (i64, u32) {
if v < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be possible to simplify this using builtin div_euclid / rem_euclid functions

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me try if it works. Thanks @jhorstmann

let v = -v;
let mut seconds = v / base;
let mut part = v % base;

if part > 0 {
seconds += 1;
part = base - part;
}
(-seconds, part as u32)
} else {
(v / base, (v % base) as u32)
}
}

/// converts a `i64` representing a `duration(s)` to [`Duration`]
#[inline]
pub fn duration_s_to_duration(v: i64) -> Duration {
Expand All @@ -154,3 +179,83 @@ pub fn duration_us_to_duration(v: i64) -> Duration {
pub fn duration_ns_to_duration(v: i64) -> Duration {
Duration::nanoseconds(v)
}

#[cfg(test)]
mod tests {
use crate::temporal_conversions::{
date64_to_datetime, split_second, timestamp_ms_to_datetime,
timestamp_ns_to_datetime, timestamp_us_to_datetime, NANOSECONDS,
};
use chrono::NaiveDateTime;

#[test]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth a test of an exact number of seconds, to check the branch in split_seconds?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I will add a few more tests.

fn negative_input_timestamp_ns_to_datetime() {
assert_eq!(
timestamp_ns_to_datetime(-1),
NaiveDateTime::from_timestamp(-1, 999_999_999)
);

assert_eq!(
timestamp_ns_to_datetime(-1_000_000_001),
NaiveDateTime::from_timestamp(-2, 999_999_999)
);
}

#[test]
fn negative_input_timestamp_us_to_datetime() {
assert_eq!(
timestamp_us_to_datetime(-1),
NaiveDateTime::from_timestamp(-1, 999_999_000)
);

assert_eq!(
timestamp_us_to_datetime(-1_000_001),
NaiveDateTime::from_timestamp(-2, 999_999_000)
);
}

#[test]
fn negative_input_timestamp_ms_to_datetime() {
assert_eq!(
timestamp_ms_to_datetime(-1),
NaiveDateTime::from_timestamp(-1, 999_000_000)
);

assert_eq!(
timestamp_ms_to_datetime(-1_001),
NaiveDateTime::from_timestamp(-2, 999_000_000)
);
}

#[test]
fn negative_input_date64_to_datetime() {
assert_eq!(
date64_to_datetime(-1),
NaiveDateTime::from_timestamp(-1, 999_000_000)
);

assert_eq!(
date64_to_datetime(-1_001),
NaiveDateTime::from_timestamp(-2, 999_000_000)
);
}

#[test]
fn test_split_seconds() {
let (sec, nano_sec) = split_second(100, NANOSECONDS);
assert_eq!(sec, 0);
assert_eq!(nano_sec, 100);

let (sec, nano_sec) = split_second(123_000_000_456, NANOSECONDS);
assert_eq!(sec, 123);
assert_eq!(nano_sec, 456);

let (sec, nano_sec) = split_second(-1, NANOSECONDS);
assert_eq!(sec, -1);
assert_eq!(nano_sec, 999_999_999);

let (sec, nano_sec) = split_second(-123_000_000_001, NANOSECONDS);
assert_eq!(sec, -124);
assert_eq!(nano_sec, 999_999_999);
}
}