Replace iso8601-duration with iso8601 crate

[LukasKalbertodt]

Pull Request

Related issue

Fixes #<issue_number>

What does this PR do?

Replace iso8601-duration with iso8601 crate.

For one, this updates the indirect dependency nom from 5.x to 7.x, which gets rid of a future incompatibility warning produced by the compiler:

warning: the following packages contain code that will be rejected by a future version of Rust: nom v5.1.2
note: to see what the problems were, use the option `--future-incompat-report`, or run `cargo report future-incompatibilities --id 67`

But to achieve that I could have updated iso8601-duration to 0.2. However, the 0.2 version makes conversion into an std::time::Duration weirdly hard. And I'm also confused by the fields being f32 for no reason I can think of. Additionally, the crate has very few users and there is no changelog. So in general, the iso8601 crate looks much better. Even if it gives us date parsing, which we don't need, I think it's still a better option.

PR checklist

Please check if your PR fulfills the following requirements:

• Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
• Have you read the contributing guidelines?
• Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

[irevoire]

Hey, yeah, that looks like a good idea! Thanks for your research

[LukasKalbertodt]

The CI failure doesn't seem to have anything to do with my changes, right?

[bidoubiwa]

bors try

[bors]

try

Build failed:

• integration-tests

[bidoubiwa]

This is weird, the CI seems to run properly on the other PR's. What happens if you run your test locally?

[LukasKalbertodt]

Oh sorry, should have replied here sooner. Yes the CI failure is my fault. When I sent my last comment I was only on phone. But now I could reproduce the problem locally.

The problem is that the new crate iso8601 only supports up to millisecond precision, whereas the old one stored seconds in f32, so supported ... let's say "more" precision (floats are weird). I adjusted the test now to only check for milliseconds. But of course that might not be ideal/intended. Do you think millisecond-precision is enough for this use case? If yes, then this can be merged.

If not, then... oof. So iso8601-duration is, as I said, not ideal. Its to_std will now return None if a month or year is specified in the duration, despite std::time::Duration being able to represent those. Further, it uses f32 for everything, which means it's possible to lose precision in some places (compare: std::time::Duration has 128 bits). Ideally the time crate should offer ISO 8601 parsing of durations. That crate is already used here for parsing dates. Might be worth it to create an issue there (edit: I did). But yeah, as I see it, there is no optimal solution right now. Pick your poison.

[irevoire]

Hey, I don't think it helps but just fyi, here is the code we use in meilisearch to serialize the duration; https://github.com/meilisearch/meilisearch/blob/fb9d9239b2113f21747b64f6c64f1d3014ed79c2/meilisearch-types/src/tasks.rs#L509-L558

It initially comes from time 0.2.

[LukasKalbertodt]

Mh okay, so sub-millisecond durations are serialized. So if this library would just store milliseconds, that would mean a loss in precision. That said, with the old library and seconds: f32, the precision is also not always great. See this playground. Around 1s, the precision is in the hundreds of nanoseconds. But with 24h durations, it's only in the tens of milliseconds (i.e. worse than the new crate).

Again, neither is really good, but it's your call whether millisecond precision suffices for this Rust integration. (Speaking as a user of the library: while I would probably think "oh gosh, why the heck does the integration give me worse precision?", I don't need higher precision. I can't imagine someone needing more than ms precision.)

If you prefer a solution without loss in precision, I can also hand code a deserialization function in this library. Might not be that terribly difficult. But also see the linked time issue: they imply that parsing an ISO8601 duration to the std or time Duration doesn't make that much sense. Honestly, I don't even know anymore what a proper solution would look like. Edit: nevermind that.

[bidoubiwa]

Hey @LukasKalbertodt

We discussed it and decided that your suggestion is a good enhancement :) Thanks a lot for it 🙏
I'm approving it!

[bidoubiwa]

bors merge

[bors]

Build succeeded:

• clippy-check
• integration-tests
• readme-check
• rust-format
• wasm-build