-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement serialization/de-serialization support for chrono::Duration
#4037
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
4dc3359
Added `ToSql` and `FromSql` support
McDic 689f594
Add docs
McDic 8e4f1a2
Added proxy and fixed scaling
McDic c572c96
Added tests
McDic 06b7c98
Improved test to use correct query strings
McDic e434baa
Use old version compatible interface on `chrono::Duration`
McDic 1392af3
Make another compatibility patch(avoid `Duration::new`)
McDic 7504257
Reduced duplicated code
McDic e51f170
Add much more testcases
McDic 5c165cd
Fix rustdoc
McDic File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that seems arbitrary (although it's probably the most sensible value if there has to be any)
Since we already have a type that represents pg's interval, maybe what we'd want instead is to propose typed conversion functions from that to chrono, which would make very clear that there's this going on and that the roundtrip is not expected to preserve the values and that adding the Rust types is not expected to produce the same results as adding on the pg side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please read line 175~178. This is a ratio when the Postgres explicitly converts between months and days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree these are the most sensible values.
What I mean is that
'2024-05-25'::Timestamp + '1 month'::interval
!=DateTime::from_sql('2024-05-25'::Timestamp) + chrono::TimeDelta::from_sql('1 month'::interval)
, andToSql(chrono::TimeDelta::from_sql('1 month'::interval)) != '1 month'::interval
, and I'm wondering if this behavior doesn't make for enough of a trap that we should instead have users deserialize toPgInterval
and just provide functions that make conversions from that tochrono::TimeDelta
reasonably easy but more explicit on that it may lose information and behave differently.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know there isn't any separated dimension or field for months in
chrono::Duration
. I don't think any other widely used Rust "timedelta" types support it. It is fundamental behavior difference between PostgreSQL interval and widely used (and standard in my opinion) interval data types.PostgreSQL returns timedelta with "0 month" when it is produced by
datetime - datetime
. For exampleSELECT '2024-06-26'::Timestamp - '2024-05-26'::Timestamp
will return'31 days'::interval
instead of'1 month'::interval
. The month field is used only when the user explicitly specifies it(would be specifying SQL literals for diesel users), so the error case you mentioned occurs on only who uses "month" dimension in their SQL query explicitly.I am not sure if there would be many cases which uses month field explicitly, as PostgreSQL also warns these special behaviors based on political stuffs like daylight savings in their official documentation. In my opinion all users who use "month" dimension in interval should know their different behaviors.
Also this PR does not delete usage of
PgInterval
, so some sensitive users may still use it if they want. However I agree it makes sense to document there could be some information loss.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@McDic Could you open a PR for the documentation clarification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weiznich Sure. I will create a PR. Is this urgent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #4051 !