Skip to content

Remove check for DurationExceedsTimestamp #1403

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

Merged
pitdicker merged 3 commits into from
Feb 7, 2024
Merged

Remove check for DurationExceedsTimestamp #1403

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f688369
Remove check for DurationExceedsTimestamp
joroKr21 Feb 2, 2024
3f780ce
Replace DurationExceedsTimestamp example with a note.
joroKr21 Feb 2, 2024
ac28866
Add a test for duration_round close to min / max
joroKr21 Feb 2, 2024
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: 40 additions & 15 deletions src/round.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,6 @@ where
return Err(RoundingError::DurationExceedsLimit);
}
let stamp = naive.timestamp_nanos_opt().ok_or(RoundingError::TimestampExceedsLimit)?;
if span > stamp.abs() {
return Err(RoundingError::DurationExceedsTimestamp);
}
Copy link
Collaborator

@pitdicker pitdicker Feb 2, 2024

Choose a reason for hiding this comment

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

If you remove this check, I expect it will introduce a panic if the date is large and the duration is even larger. Say a date in the year 200.000 and rounding to a duration of 300.000 years.

Could you add a test for this case, and do something to avoid the panic?

Copy link
Contributor Author

@joroKr21 joroKr21 Feb 2, 2024

Choose a reason for hiding this comment

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

We can't really work with such big timestamps here. It will return a TimestampExceedsLimit error:

An i64 with nanosecond precision can span a range of ~584 years. This function returns None on an out of range NaiveDateTime.
The dates that can be represented as nanoseconds are between 1677-09-21T00:12:43.145224192 and 2262-04-11T23:47:16.854775807.

Copy link
Collaborator

@pitdicker pitdicker Feb 2, 2024

Choose a reason for hiding this comment

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

Sorry, that's what I get when writing this without testing 😄. Any other errors with large durations such as 400 years that the previous check would have protected against?

Copy link
Contributor Author

@joroKr21 joroKr21 Feb 5, 2024

Choose a reason for hiding this comment

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

I couldn't find any. Added a test for timestamps that are close to the min / max of nanosecond precision

Copy link
Collaborator

@pitdicker pitdicker Feb 5, 2024

Choose a reason for hiding this comment

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

Thank you 👍. @djc will also want to have a look, but it seems good to merge for me.

if span == 0 {
return Ok(original);
}
Expand Down Expand Up @@ -216,9 +213,6 @@ where
return Err(RoundingError::DurationExceedsLimit);
}
let stamp = naive.timestamp_nanos_opt().ok_or(RoundingError::TimestampExceedsLimit)?;
if span > stamp.abs() {
return Err(RoundingError::DurationExceedsTimestamp);
}
Copy link
Collaborator

@pitdicker pitdicker Feb 2, 2024

Choose a reason for hiding this comment

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

Removing this check seems fine to me.

let delta_down = stamp % span;
match delta_down.cmp(&0) {
Ordering::Equal => Ok(original),
Expand All @@ -237,15 +231,7 @@ where
pub enum RoundingError {
/// Error when the TimeDelta exceeds the TimeDelta from or until the Unix epoch.
///
/// ``` rust
/// # use chrono::{DurationRound, TimeDelta, RoundingError, TimeZone, Utc};
/// let dt = Utc.with_ymd_and_hms(1970, 12, 12, 0, 0, 0).unwrap();
///
/// assert_eq!(
/// dt.duration_round(TimeDelta::days(365)),
/// Err(RoundingError::DurationExceedsTimestamp),
/// );
/// ```
/// Note: this error is not produced anymore.
DurationExceedsTimestamp,

/// Error when `TimeDelta.num_nanoseconds` exceeds the limit.
Expand Down Expand Up @@ -769,4 +755,43 @@ mod tests {
let span = TimeDelta::nanoseconds(-9_223_372_036_854_771_421);
assert_eq!(dt.duration_round(span), Err(RoundingError::DurationExceedsLimit));
}

#[test]
fn test_duration_trunc_close_to_epoch() {
let span = TimeDelta::minutes(15);

let dt = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap().and_hms_opt(0, 0, 15).unwrap();
assert_eq!(dt.duration_trunc(span).unwrap().to_string(), "1970-01-01 00:00:00");

let dt = NaiveDate::from_ymd_opt(1969, 12, 31).unwrap().and_hms_opt(23, 59, 45).unwrap();
assert_eq!(dt.duration_trunc(span).unwrap().to_string(), "1969-12-31 23:45:00");
}

#[test]
fn test_duration_round_close_to_epoch() {
let span = TimeDelta::minutes(15);

let dt = NaiveDate::from_ymd_opt(1970, 1, 1).unwrap().and_hms_opt(0, 0, 15).unwrap();
assert_eq!(dt.duration_round(span).unwrap().to_string(), "1970-01-01 00:00:00");

let dt = NaiveDate::from_ymd_opt(1969, 12, 31).unwrap().and_hms_opt(23, 59, 45).unwrap();
assert_eq!(dt.duration_round(span).unwrap().to_string(), "1970-01-01 00:00:00");
}

#[test]
fn test_duration_round_close_to_min_max() {
let span = TimeDelta::nanoseconds(i64::MAX);

let dt = NaiveDateTime::from_timestamp_nanos(i64::MIN / 2 - 1).unwrap();
assert_eq!(dt.duration_round(span).unwrap().to_string(), "1677-09-21 00:12:43.145224193");

let dt = NaiveDateTime::from_timestamp_nanos(i64::MIN / 2 + 1).unwrap();
assert_eq!(dt.duration_round(span).unwrap().to_string(), "1970-01-01 00:00:00");

let dt = NaiveDateTime::from_timestamp_nanos(i64::MAX / 2 + 1).unwrap();
assert_eq!(dt.duration_round(span).unwrap().to_string(), "2262-04-11 23:47:16.854775807");

let dt = NaiveDateTime::from_timestamp_nanos(i64::MAX / 2 - 1).unwrap();
assert_eq!(dt.duration_round(span).unwrap().to_string(), "1970-01-01 00:00:00");
}
}