Replace `chrono` with `time` #1307

uklotzde · 2022-03-09T16:46:56Z

BREAKING API CHANGE: The type DateTime is no longer (re-)exported. Instead you need to use time::OffsetDateTime explicitly.

Remarks:

Please check the error handling and mappings. time returns results instead of panicking like chrono does. For the FastValue impl I had to use `.expect("valid UNIX timestamp").
I had to adjust some debug strings in tests that are used for matching test results. Copy&paste from the failed test outputs. Not very handy, please verify.

TODO:

Add changelog entry

src/error.rs

src/fastfield/readers.rs

src/lib.rs

fulmicoton

Awesome! Thank you!

Can you re-add the re-export or make the case against it on the ticket? I commented there?
Also can you add your contribution on CHANGELOG.md. Don't forget to put your github username.
Make sure to highlight the breaking changes:
chrono -> time
timestamp -> unix_timestamp <---- I'm not sure that one is useful tbh.

codecov-commenter · 2022-03-10T01:51:15Z

Codecov Report

Merging #1307 (ef80c27) into main (46d5de9) will decrease coverage by 0.02%.
The diff coverage is 82.24%.

@@            Coverage Diff             @@
##             main    #1307      +/-   ##
==========================================
- Coverage   94.25%   94.22%   -0.03%     
==========================================
  Files         232      232              
  Lines       40801    40856      +55     
==========================================
+ Hits        38457    38498      +41     
- Misses       2344     2358      +14

Impacted Files	Coverage Δ
query-grammar/src/query_grammar.rs	`99.64% <ø> (ø)`
src/error.rs	`22.03% <0.00%> (-2.50%)`	⬇️
src/fastfield/multivalued/mod.rs	`96.96% <ø> (ø)`
src/fastfield/readers.rs	`86.66% <ø> (ø)`
src/query/more_like_this/more_like_this.rs	`67.54% <0.00%> (ø)`
src/schema/document.rs	`95.48% <ø> (ø)`
src/schema/term.rs	`84.45% <50.00%> (ø)`
src/lib.rs	`97.61% <75.75%> (-1.21%)`	⬇️
src/collector/histogram_collector.rs	`98.99% <83.33%> (-0.51%)`	⬇️
src/schema/field_type.rs	`84.04% <84.00%> (+0.70%)`	⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46d5de9...ef80c27. Read the comment docs.

uklotzde · 2022-03-10T10:28:47Z

I decided against introducing a type alias DateTime for time::OffsetDateTime. The code is more concise and less confusing when explicitly using the actual types from crate::time. Re-exporting the crate as a module should be sufficient.

uklotzde · 2022-03-10T12:13:51Z

@fulmicoton I am still unsure if it would be better to use PrimitiveDateTime instead of OffsetDateTime in the public API? Since the time zone information gets lost when storing an OffsetDateTime I consider PrimitiveDateTime more appropriate. The time zone UTC is implicit and only matters for the internal integer conversions.

uklotzde · 2022-03-10T13:20:59Z

I am convinced that storing PrimitiveDateTime instead of OffsetDateTime is the right choice and prevents inconsistent behavior.

For convenience impl From<OffsetDateTime> for Value is provided that performs the conversion to UTC. In the opposite direction you have to invoke PrimitiveDateTime::assume_utc() explicitly to get back an OffsetDateTime.

Please verify: 304013f

fulmicoton · 2022-03-11T04:53:21Z

I am convinced that storing PrimitiveDateTime instead of OffsetDateTime is the right choice and prevents inconsistent behavior.

Haha, I noticed that yesterday before going to sleep
I'm glad you already made the change.

CHANGELOG.md

uklotzde · 2022-03-13T12:18:05Z

Converted to draft because there are still some API inconsistencies that need to be resolved. Moreover all time zone conversions should be explicit, not implicit.

fulmicoton · 2022-03-14T04:56:53Z

@uklotzde Understood! Please let me know when I should resume the review.

uklotzde · 2022-03-14T09:11:09Z

@fulmicoton Done. Please see the commit messages for a summary of the changes.

I would also like to drop the convenient but lossy impl From<OffsetDateTime> for Value and replace it with an explicit conversion function Value::date_utc() that resembles the newly added Document::add_date_utc(). But unfortunately that would break the doc! macro for OffsetDateTime (e.g. used by many tests) and I don't know how to fix that easily.

uklotzde · 2022-03-14T09:46:26Z

A simple extension of the time API would serve us well: time-rs/time#454

uklotzde · 2022-03-14T20:22:21Z

More and more I come to the conclusion that using a dedicated newtype with conversion functions would be the correct approach. Using PrimitiveDateTime/OffsetDateTime is unsound because storing those values is lossy by implicitly reducing the precision to seconds.

Including conversions from/to OffsetDateTime and PrimitiveDateTime.

uklotzde · 2022-03-15T00:04:56Z

Phew, that escalated quickly.

Fortunately, introducing the newtype revealed how scattered the conversion logic was. All conversions are now implemented by dedicated functions of DateTime.

uklotzde · 2022-03-15T10:35:32Z

Ready again. Hopefully I didn't mess anything up. Especially the FastValue conversions to_u64() and as_64() are easy to confuse. A less generic and more distinctive naming would help.

fulmicoton · 2022-03-16T03:08:03Z

src/lib.rs

+/// functions and not by implementing any `From`/`Into` traits
+/// to prevent unintended usage.
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub struct DateTime {


Can we have a nicer implem of Debug showing Rfc3339?

Just the timestamp 2019-10-12T05:20:50.52Z or like DateTime { 2019-10-12T05:20:50.52Z }?

Just the date is ok I think.

d6b46a3

The documentation of the DateTime wrapper now mentions the internal UTC representation, because that leaks not only for debugging but also when serializing as a JSON value.

I have noticed that the query parser is not tested with explicit time zone offsets, only with UTC? But that's unrelated and a different task.

fulmicoton · 2022-03-16T03:12:09Z

I agree with your change (introducing a DateTime object)... Chrono was nice for this.

uklotzde · 2022-03-18T11:14:32Z

I agree with your change (introducing a DateTime object)... Chrono was nice for this.

Nevertheless using the chrono type directly was unsound because of the precision loss when storing those values.

fulmicoton · 2022-03-21T01:48:06Z

Thank you @uklotzde for the very solid work.

uklotzde · 2022-03-21T10:58:34Z

@fulmicoton Please double check once again that I didn't miss anything. Especially, if the explicit truncation to seconds inside the newtype is viable and doesn't break existing use cases. Formerly this truncation code was scattered at various places. I am still not completely familiar with when and how values are transformed and stored.

fulmicoton · 2022-03-22T00:43:22Z

It should be ok @uklotzde

For date values `chrono` has been replaced with `time` - The `time` crate is re-exported as `tantivy::time` instead of `tantivy::chrono`. - The type alias `tantivy::DateTime` has been removed. - `Value::Date` wraps `time::PrimitiveDateTime` without time zone information. - Internally date/time values are stored as seconds since UNIX epoch in UTC. - Converting a `time::OffsetDateTime` to `Value::Date` implicitly converts the value into UTC. If this is not desired do the time zone conversion yourself and use `time::PrimitiveDateTime` directly instead. Closes #1304

Replace chrono with time

841d0c2