How to round a Timestamp to the nearest SignedDuration?

[vultix]

What's the easiest way to round a Timestamp to the nearest SignedDuration?

This is what I'm currently doing:

let bucket_start = timestamp
    .round(
        TimestampRound::new()
            .smallest(Unit::Nanosecond)
            .mode(RoundMode::Trunc)
            .increment(bucket_duration.as_nanos().try_into().unwrap()),
    )
    .unwrap();

I was somewhat expecting to be able to use the mod operator to do this

let bucket_start = timestamp - (timestamp % bucket_duration);

Another option I've considered:

let timestamp_ns = new_timestamp.as_nanosecond();
let bucket_start_ns = timestamp_ns - (timestamp_ns % self.bucket_duration.as_nanos());
let bucket_start = Timestamp::from_nanosecond(bucket_start_ns).unwrap();

[BurntSushi]

You could remove .smallest(Unit::Nanosecond), since that's the default. But what you have looks reasonable to me? Is there a specific problem you're facing? I note that I think it makes the actual intent of the code much clearer than the code written with modulus. Also, by virtue of using RoundMode::Trunc, it also makes it clear how the value is actually rounded. Where as when you use modulus, there are two different reasonable approaches (i64::rem and i64::rem_euclid) that will result in different values for a negative timestamp (pre-1970).

[vultix]

What I have now is working just fine, but was a bit unintuitive to find. Adding helper methods to create a TimestampRound from a SignedDuration would be nice.

TimestampRound::from(bucket_duration).mode(RoundMode::Trunc)

[BurntSushi]

Jiff currently doesn't have any From impls that only accept the increment. There is impl From<(Unit, i64)> for TimestampRound. It's probably reasonable to add impl From<(Unit, SignedDuration)> for TimestampRound too. Adding an impl only for the increment I fear might be a touch too implicit.

With that said, I think the reason why this is probably a bad idea is that the units of a SignedDuration are always nanoseconds. But the units of the rounding increment are tied to the smallest unit given. This works fine when Unit::Nanosecond is used, but it doesn't really make sense otherwise. I fear this could lead to some subtle footguns. You could say it makes sense because it just means that constructing a TimestampRound from a SignedDuration also means setting the smallest unit to Unit::Nanosecond. But if you then change the smallest unit to something else, the meaning of the increment given becomes somewhat ambiguous. Does it stay the same as an absolute value? Or does it actually change based on the units given?

[BurntSushi]

I am much more open to adding more docs to help guide folks. Is there a specific spots in the docs where something like this could be called out that you would have seen?

[vultix]

I think documentation on the TimestampRound type that gives this as an example would be great for discoverability!

The only thing I don't love about this user experience is that TimestampRound::increment takes an i64, whereas SignedDuration::as_nanos gives an i128. This forces the user to downcast, which is error prone. I worry most would simply do as i64 instead of .try_into(), which can silently introduce bugs.

An explicit TimestampRound::from_duration associated method may be better than the From impl, as it can handle this error seamlessly, and document when it may error.

[BurntSushi]

But TimestampRound::from_duration still has the same problem I cited above. If you do TimestampRound::from_duration(...).smallest(Unit::Microsecond), what is its behavior?

And note that there are already a number of requirements on what increment values are considered legal. An error will occur if you get it wrong. This doesn't quite handle the error case of signed_duration.as_nanos() as i64 silently truncating though. But since the increment already has to divide evenly into 86,400 seconds, and 86,400,000,000,000 nanoseconds fits into an i64, it's hard for me to reason about exactly what could go wrong here.

In any case, as is a footgun unto itself. I'm not sure I want to be adding APIs to Jiff that could potentially have their own footguns just to mitigate as.

I'll think about the documentation problem. Maybe just adding your use case as an example on Timestamp::round would be good.