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

Split timestamp_s_to_datetime to date and time to avoid unnecessary computation #6755

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from 2 commits
Commits
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
55 changes: 55 additions & 0 deletions arrow-array/src/temporal_conversions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,18 @@ pub const NANOSECONDS_IN_DAY: i64 = SECONDS_IN_DAY * NANOSECONDS;
/// Number of days between 0001-01-01 and 1970-01-01
pub const EPOCH_DAYS_FROM_CE: i32 = 719_163;

/// Constant from chrono crate
///
/// Number of days between Januari 1, 1970 and December 31, 1 BCE which we define to be day 0.
/// 4 full leap year cycles until December 31, 1600 4 * 146097 = 584388
/// 1 day until January 1, 1601 1
/// 369 years until Januari 1, 1970 369 * 365 = 134685
/// of which floor(369 / 4) are leap years floor(369 / 4) = 92
/// except for 1700, 1800 and 1900 -3 +
/// --------
/// 719163
pub const UNIX_EPOCH_DAY: i64 = 719_163;
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks to be identical to the EPOCH_DAYS_FROM_CE const above, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems that EPOCH_DAYS_FROM_CE is actually not used, remove it. To keep the variable name consistent to chrono


/// converts a `i32` representing a `date32` to [`NaiveDateTime`]
#[inline]
pub fn date32_to_datetime(v: i32) -> Option<NaiveDateTime> {
Expand Down Expand Up @@ -134,6 +146,31 @@ pub fn timestamp_s_to_datetime(v: i64) -> Option<NaiveDateTime> {
Some(DateTime::from_timestamp(v, 0)?.naive_utc())
}

/// Similar to timestamp_s_to_datetime but only compute `date`
#[inline]
pub fn timestamp_s_to_date(secs: i64) -> Option<NaiveDateTime> {
let days = secs.div_euclid(86_400) + UNIX_EPOCH_DAY;
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we're ignoring leap seconds here?

if days < i32::MIN as i64 || days > i32::MAX as i64 {
return None;
}
let date = NaiveDate::from_num_days_from_ce_opt(days as i32)?;
Some(date.and_time(NaiveTime::default()).and_utc().naive_utc())
}

/// Similar to timestamp_s_to_datetime but only compute `time`
#[inline]
pub fn timestamp_s_to_time(secs: i64) -> Option<NaiveDateTime> {
let secs = secs.rem_euclid(86_400);
Copy link
Contributor

Choose a reason for hiding this comment

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

and ignoring leap seconds here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean leap second? The code is copied from chrono I prefer to keep than consistent if possible. If the computation is not expected we should fix it in chrono

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose that depends on whether the place where this came from is supposed to respect leap seconds, etc or not. It's not technically correct when using UTC, see https://en.wikipedia.org/wiki/Leap_second for a list of leap seconds since unix epoch. It's also not going to be far off either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's support it when chrono did so.

let time = NaiveTime::from_num_seconds_from_midnight_opt(secs as u32, 0)?;
Some(
DateTime::<Utc>::from_naive_utc_and_offset(
NaiveDateTime::new(NaiveDate::default(), time),
Utc,
)
.naive_utc(),
)
}

/// converts a `i64` representing a `timestamp(ms)` to [`NaiveDateTime`]
#[inline]
pub fn timestamp_ms_to_datetime(v: i64) -> Option<NaiveDateTime> {
Expand Down Expand Up @@ -274,10 +311,28 @@ pub fn as_duration<T: ArrowPrimitiveType>(v: i64) -> Option<Duration> {
mod tests {
use crate::temporal_conversions::{
date64_to_datetime, split_second, timestamp_ms_to_datetime, timestamp_ns_to_datetime,
timestamp_s_to_date, timestamp_s_to_datetime, timestamp_s_to_time,
timestamp_us_to_datetime, NANOSECONDS,
};
use chrono::DateTime;

#[test]
fn test_timestamp_func() {
let timestamp = 1234;
let datetime = timestamp_s_to_datetime(timestamp).unwrap();
let expected_date = datetime.date();
let expected_time = datetime.time();

assert_eq!(
timestamp_s_to_date(timestamp).unwrap().date(),
expected_date
);
assert_eq!(
timestamp_s_to_time(timestamp).unwrap().time(),
expected_time
);
}

#[test]
fn negative_input_timestamp_ns_to_datetime() {
assert_eq!(
Expand Down
Loading