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

Serde support for millisecond precision timestamps is broken by the lack of truncating i128 to i64. #721

Open
JohnDowson opened this issue Jan 9, 2025 · 5 comments
Labels
A-third-party Area: implementations of traits from other crates C-enhancement Category: an enhancement with existing code

Comments

@JohnDowson
Copy link

Causes "i128 is not supported" runtime errors when used with serde_json and/or bson.

let timestamp = datetime.unix_timestamp_nanos() / 1_000_000;

serde-rs/json#846

@jhpratt
Copy link
Member

jhpratt commented Jan 13, 2025

While the serde_json issue was closed, I don't see how this isn't an issue on their end. A Unix timestamp in milliseconds, microseconds, or nanoseconds cannot be stored in a 64 bit integer. As far as I can tell this is working as intended; the serializer is where the issue lies.

@JohnDowson
Copy link
Author

Can it not? Storing millisecond offset from 01.01.1970 as an i64 should be totally fine for another 200 years, if I'm mathing this correctly.

@jhpratt
Copy link
Member

jhpratt commented Jan 13, 2025

The supported range of years is ±9999 by default and ±999,999 with the large-dates feature enabled.

@JohnDowson
Copy link
Author

JohnDowson commented Jan 13, 2025

Though I do agree that not supporting 128 bit integers, and offering to jump straight to bigint as a solution is one of choices of all time.

In the use case where this came up for us, we're writing timestamps to Mongo, which doesn't support 128 bit integers either.
And if you think about it, if serde_json supported 128 bit integers by default, it would unexpectedly break were that 128 bit integer get deserialized by a JavaScript consumer.

@jhpratt
Copy link
Member

jhpratt commented Jan 13, 2025

So…math-wise, 2⁶³ milliseconds is larger than the 1 million supported. As such it was an oversight on my end when this was first implemented. This does not apply to microseconds and nanoseconds, which would be 292,000 years and 292 years respectively.

I'd accept a pull request that duplicates the time::serde::timestamp::milliseconds module as time::serde::timestamp::milliseconds_i64, truncating the value (using num-conv's methods) when serializing and requesting an i64 when deserializing (and similarly calling .extend()).

@jhpratt jhpratt added C-enhancement Category: an enhancement with existing code A-third-party Area: implementations of traits from other crates labels Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-third-party Area: implementations of traits from other crates C-enhancement Category: an enhancement with existing code
Projects
None yet
Development

No branches or pull requests

2 participants