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

Saturating operations #1579

Open
bragov4ik opened this issue May 10, 2024 · 11 comments
Open

Saturating operations #1579

bragov4ik opened this issue May 10, 2024 · 11 comments

Comments

@bragov4ik
Copy link
Contributor

saturating_* methods might be quite useful. They are present in standard library for primitive types, so the use case can be quite frequent (?).

I found only one mention of saturation in this repo: "...library users could implement some saturating_* methods...".

Since there seems to be no separate discussion about this, I suggest to consider this implementation here.

@bragov4ik
Copy link
Contributor Author

I encountered a case where I need to add Duration to NativeDate with saturation. There's a function that generates intervals of dates (NativeDate), with some step (Duration).
It makes sense to have step = Duration::max_value() if the caller wants a single interval. Saturating add seemd quite helpful there.

pub fn generate_date_ranges(
    start: NaiveDate,
    end: NaiveDate,
    step: Duration,
) -> Vec<RangeInclusive<NaiveDate>> {
    let mut date_range = Vec::new();
    let mut next_start = start;
    while next_start < end {
        let next_end = next_start.saturating_add(step);
        date_range.push(RangeInclusive::new(next_start, next_end));
        next_start = next_end;
    }
    date_range
}

@djc
Copy link
Member

djc commented May 12, 2024

A single person requesting this feature is not enough, in this case, to add this feature, I think. Let's keep this open and see if other people turn up that want/need this?

@pitdicker
Copy link
Collaborator

For integers saturation operations make sense because they have a commonly agreed-upon range.
For dates and datetimes the range we support is somewhat arbitrary, and min and max are way outside the sensible range of dates. Saturating operations would make the min and max values more prominent, and I don't think that is good thing.

@Veetaha
Copy link

Veetaha commented May 28, 2024

Hey @djc. I'm writing a code that needs to pass DateTime<Utc> as a filter to a DB request to specify a time range for the calculation to be limited to the past 30 days. I don't see any good way to handle the None case of Utc::now().checked_sub_days(chrono::Days::new(30)). I just want a date that is exactly 30 days from now (or lower if at the edge of timestamp starting points). How can I know if there were any time gaps so that I could skip them? How should I skip them? I think a saturating subtraction method would need to handle this.

Maybe I landed on a wrong issue, but the main comment is that I'd like a method that gracefully handles gaps in time and skips them when doing subtraction even if it doesn't saturate at the bounds of allowed date-time values. I can just use a default DateTime::MIN_UTC instead. In fact, I think checked_sub method should return a LocalResult instead of option so that I could act upon timestamps ambiguity or out-of-range errors but it should still handle gaps.

@pitdicker
Copy link
Collaborator

How can I know if there were any time gaps so that I could skip them?

If the time zone is UTC there are no gaps, Utc::now().checked_sub_days(chrono::Days::new(30)) will always succeed unless your pc clock is set 250.000+ years into the past. Or were you thinking of another cause?

If the time zone is Local or something else that supports DST it is best to work with methods that return LocalResult, en we will use it as the return value of more methods in chrono 0.5. Let me need if I should write an example.

@djc
Copy link
Member

djc commented May 28, 2024

@pitdicker was thinking about this: if the result of a calculation is None, IIRC that means it falls into a gap. I read the previous comment as wondering if there was a way to get at the gap boundaries, which seems like a reasonable request. Do we have something like that today? (I don't think so.) How hard do you think that would be to add?

@pitdicker
Copy link
Collaborator

Local::now().naive_local().checked_sub_days(chrono::Days::new(30))?.and_local_timezone(Local) should do the same and return a LocalResult. earliest() and latest() can then be used for the gap boundaries.

Quoting from #1448:

Methods on DateTime currently don't return a LocalResult, but consider any result that falls in a timezone transition as an error. This makes it cumbersome for users to handle these cases correctly. They have to work on a NaiveDateTime and then convert it back with TimeZone::from_local_datetime. Returning a LocalResult would be a big improvement for correctness in my opinion.

Sorry for being absent again. I really need to continue working on this for 0.5...

@Veetaha
Copy link

Veetaha commented May 28, 2024

If the time zone is UTC there are no gaps

Oh, right... I was confused by the method signature and docs mentioning gaps. It is indeed basically infallible in UTC, so there is no problem in my case

@djc
Copy link
Member

djc commented May 30, 2024

Local::now().naive_local().checked_sub_days(chrono::Days::new(30))?.and_local_timezone(Local) should do the same and return a LocalResult. earliest() and latest() can then be used for the gap boundaries.

earliest() and latest() don't actually return gap boundaries, right? They only yield a value for Single and Ambiguous which I suppose don't apply here.

@tqwewe
Copy link

tqwewe commented Oct 30, 2024

Saturating add/sub would be great in my case.

For my use case, I have a "stale duration" variable, where a piece of data is only considered valid if it's not stale. However, in some cases I don't want to have a stale duration, and whilst I could wrap it in an option, DateTime::MAX_UTC would be a great fallback. However if I do my_timestamp + stale_duration < now, I may get an error due to the resulting timestamp being too high. my_timestamp.saturating_add(stale_duration) < now would be perfect.

@djc
Copy link
Member

djc commented Nov 5, 2024

Suggest you submit a PR with any suggestions you have if they fit into the current API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants