Remove check for DurationExceedsTimestamp #1403

joroKr21 · 2024-02-02T12:55:03Z

This check is not necessary and prevented rounding and truncation from working correctly on timestamps close to the Unix epoch.

fixes #1375

Thanks for contributing to chrono!

If your feature is semver-compatible, please target the main branch;
for semver-incompatible changes, please target the 0.5.x branch.

Please consider adding a test to ensure your bug fix/feature will not break in the future.

codecov · 2024-02-02T13:05:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ef9a4c9) 91.85% compared to head (377fe51) 91.86%.

❗ Current head 377fe51 differs from pull request most recent head ac28866. Consider uploading reports for the commit ac28866 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1403   +/-   ##
=======================================
  Coverage   91.85%   91.86%           
=======================================
  Files          38       38           
  Lines       17526    17531    +5     
=======================================
+ Hits        16098    16104    +6     
+ Misses       1428     1427    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pitdicker · 2024-02-02T13:05:06Z

Thank you @joroKr21. I'll have to think this through.

src/round.rs

pitdicker · 2024-02-02T13:58:07Z

src/round.rs

@@ -220,9 +217,6 @@ where
            return Err(RoundingError::DurationExceedsLimit);
        }
        let stamp = naive.timestamp_nanos_opt().ok_or(RoundingError::TimestampExceedsLimit)?;
-        if span > stamp.abs() {
-            return Err(RoundingError::DurationExceedsTimestamp);
-        }


Removing this check seems fine to me.

pitdicker · 2024-02-02T14:00:35Z

src/round.rs

@@ -181,9 +181,6 @@ where
            return Err(RoundingError::DurationExceedsLimit);
        }
        let stamp = naive.timestamp_nanos_opt().ok_or(RoundingError::TimestampExceedsLimit)?;
-        if span > stamp.abs() {
-            return Err(RoundingError::DurationExceedsTimestamp);
-        }


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?

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.

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?

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

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

djc

Just needs a rebase.

This check is not necessary and prevented rounding and truncation from working correctly on timestamps close to the Unix epoch.

joroKr21 · 2024-02-07T10:02:27Z

Looks like CI is failing on something unrelated

pitdicker · 2024-02-07T11:24:13Z

All CI failures are with the reason:

 error[E0635]: unknown feature `stdsimd`
  --> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ahash-0.7.7/src/lib.rs:33:42
   |
33 | #![cfg_attr(feature = "stdsimd", feature(stdsimd))]
   |                                          ^^^^^^^

Curious. In any case unrelated to your work.

pitdicker · 2024-02-07T11:26:26Z

We don't seem to be the only one thin-edge/thin-edge.io#2687.

pitdicker · 2024-02-07T12:30:45Z

Rkyv seems in the process of updating to hashbrown 0.14 (rkyv/rkyv@7b88320). Hashbrown 0.14 uses the updated ahash 0.8, which should build on the current nightly. Once a new version of rkyv is released our CI should be green again.

cc @djkoloski

djkoloski · 2024-02-07T13:06:45Z

rkyv 0.7 will not be updating to a newer version of hashbrown because it would be a server breaking change. 0.8 won't be out for a while either. I did create a branch of 0.7 that updates hashbrown yesterday (0.7-hashbrown-0.14) so you could change your dependency to a git dependency for nightly CI testing if that would help. Unfortunately I don't know of a better solution to this problem besides hashbrown releasing a fixed 0.12 version without stdsimd.

djc · 2024-02-07T13:12:05Z

There's also a PR to request an update to ahash 0.7: tkaitchuck/aHash#201.

pitdicker · 2024-02-07T13:14:56Z

rkyv 0.7 will not be updating to a newer version of hashbrown because it would be a server breaking change. 0.8 won't be out for a while either.

Thank you for the explanation and replying here.

There's also a PR to request an update to ahash 0.7: tkaitchuck/aHash#201.

Nice, that seems to be the easier fix.

pitdicker reviewed Feb 2, 2024

View reviewed changes

src/round.rs Show resolved Hide resolved

joroKr21 force-pushed the bugfix/close-to-epoch branch from 8bbeaaf to 23f4944 Compare February 2, 2024 13:17

pitdicker reviewed Feb 2, 2024

View reviewed changes

joroKr21 force-pushed the bugfix/close-to-epoch branch from 5d0bbfe to 377fe51 Compare February 2, 2024 14:43

pitdicker approved these changes Feb 5, 2024

View reviewed changes

djc approved these changes Feb 7, 2024

View reviewed changes

joroKr21 added 3 commits February 7, 2024 10:55

Remove check for DurationExceedsTimestamp

f688369

This check is not necessary and prevented rounding and truncation from working correctly on timestamps close to the Unix epoch.

Replace DurationExceedsTimestamp example with a note.

3f780ce

Add a test for duration_round close to min / max

ac28866

joroKr21 force-pushed the bugfix/close-to-epoch branch from 377fe51 to ac28866 Compare February 7, 2024 09:57

pitdicker merged commit 5656a9e into chronotope:main Feb 7, 2024
30 of 35 checks passed

joroKr21 deleted the bugfix/close-to-epoch branch February 7, 2024 11:29

pitdicker mentioned this pull request Feb 10, 2024

Prepare 0.4.34 #1424

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove check for DurationExceedsTimestamp #1403

Remove check for DurationExceedsTimestamp #1403

joroKr21 commented Feb 2, 2024 •

edited

Loading

codecov bot commented Feb 2, 2024 •

edited

Loading

pitdicker commented Feb 2, 2024

pitdicker Feb 2, 2024

pitdicker Feb 2, 2024

joroKr21 Feb 2, 2024

pitdicker Feb 2, 2024

joroKr21 Feb 5, 2024

pitdicker Feb 5, 2024

djc left a comment

joroKr21 commented Feb 7, 2024

pitdicker commented Feb 7, 2024

pitdicker commented Feb 7, 2024

pitdicker commented Feb 7, 2024

djkoloski commented Feb 7, 2024

djc commented Feb 7, 2024

pitdicker commented Feb 7, 2024

Remove check for DurationExceedsTimestamp #1403

Remove check for DurationExceedsTimestamp #1403

Conversation

joroKr21 commented Feb 2, 2024 • edited Loading

Thanks for contributing to chrono!

codecov bot commented Feb 2, 2024 • edited Loading

Codecov Report

pitdicker commented Feb 2, 2024

pitdicker Feb 2, 2024

Choose a reason for hiding this comment

pitdicker Feb 2, 2024

Choose a reason for hiding this comment

joroKr21 Feb 2, 2024

Choose a reason for hiding this comment

pitdicker Feb 2, 2024

Choose a reason for hiding this comment

joroKr21 Feb 5, 2024

Choose a reason for hiding this comment

pitdicker Feb 5, 2024

Choose a reason for hiding this comment

djc left a comment

Choose a reason for hiding this comment

joroKr21 commented Feb 7, 2024

pitdicker commented Feb 7, 2024

pitdicker commented Feb 7, 2024

pitdicker commented Feb 7, 2024

djkoloski commented Feb 7, 2024

djc commented Feb 7, 2024

pitdicker commented Feb 7, 2024

joroKr21 commented Feb 2, 2024 •

edited

Loading

codecov bot commented Feb 2, 2024 •

edited

Loading