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

Fix sqlite timestamps #3694

Merged
merged 8 commits into from
Aug 10, 2023
Merged

Conversation

Sytten
Copy link
Contributor

@Sytten Sytten commented Jul 14, 2023

Fixes #3693
Fixes #3320

This is a big change in the way we store and parse timestamp in sqlite.

  • (Major) We don't ignore timezones anymore if they are present, I am unsure why this what put in place but it feels very very very wrong to me. The data written by diesel is always switched to UTC, but we can't assume the data in the database was written by us. We still fallback to a "naive" implementation if the timezone is missing. I had to remove "bad" tests since 1970-01-01 00:00+01:00 is not the same time as 1970-01-01 00:00Z in my book. We still ignore the timezone if the user uses a "naive" datetime to read the data (I think that is fair, though we could verify with other ORM if they correct to UTC instead).
  • Uniformity between time and chrono, the code looks more similar and the formats are the same so a user can switch library without any issue now (that was something I found in ToSql<TimestamptzSqlite, Sqlite> is inconsistent between NaiveDateTime and PrimitiveDateTime #3693)
  • Fixed some documentation

@Sytten Sytten force-pushed the fix/sqlite-timestamp-parsing branch from 81e0974 to 151eac8 Compare July 14, 2023 03:34
@Sytten Sytten mentioned this pull request Jul 14, 2023
1 task
@weiznich weiznich requested a review from a team July 14, 2023 12:12
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this PR. I've left a bunch of questions and notes for minor improvements. Beside of that I want to clarify an important point: You consider that a minor change, as it does only strictly extend which time formats are parsed and does not error out on formats that worked before?

Additionally I would like to see a few tests for time<->chrono roundtrips just to make sure that they stay consistent in the future.

diesel/src/sql_types/mod.rs Show resolved Hide resolved
@Sytten Sytten force-pushed the fix/sqlite-timestamp-parsing branch from e9b36ea to 6e1ce7c Compare July 26, 2023 21:27
@Sytten Sytten requested a review from weiznich July 26, 2023 21:28
@Sytten
Copy link
Contributor Author

Sytten commented Jul 26, 2023

@weiznich This is ready for another round of review, I changed a bit the encoding.

I realized we didn't need the differentiation for subsecond in chrono since the dot is handled by the formatter.
I changed time to now print all digits instead of just 3 or 6 like it currently done, this aligns it with chrono (and what sqlx is doing whatever that's worth).

I am still unsure how many digits we should really print, PG and Mysql cut at the microseconds but sqlite by default stops at the millisecond with %f. And if we should differentiate between a datetime with timezone and a "naive" datetime/time. Any external opinion would be appreciated, but it seems all over the place.

The case for printing they all is quite simple, it's still respecting the RFC3339 and the user can decide to not pass us the microseconds/nanoseconds if they don't want them in the database but the inverse (stopping at X digits) means we decide what loss of information is acceptable.

Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update and sorry for taking so long to do the review.

I think it's mostly fine now. I would like to change the comments to doc-comments as suggested my @Mingun
I also would like to actually check the values in the compatibility tests to ensure that we load the right thing there.

diesel/src/sqlite/types/date_and_time/chrono.rs Outdated Show resolved Hide resolved
diesel/src/sqlite/types/date_and_time/mod.rs Show resolved Hide resolved
diesel/src/sqlite/types/date_and_time/time.rs Outdated Show resolved Hide resolved
@Sytten
Copy link
Contributor Author

Sytten commented Aug 10, 2023

@weiznich Ready!

Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

@weiznich weiznich added this pull request to the merge queue Aug 10, 2023
@weiznich weiznich added the maybe backport Maybe backport this pr to the latest diesel release label Aug 10, 2023
Merged via the queue into diesel-rs:master with commit 48555b1 Aug 10, 2023
32 checks passed
weiznich added a commit to weiznich/diesel that referenced this pull request Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maybe backport Maybe backport this pr to the latest diesel release
Projects
None yet
3 participants