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

Implement serialization/de-serialization support for chrono::Duration #4037

Merged
merged 10 commits into from
May 24, 2024

Conversation

McDic
Copy link
Contributor

@McDic McDic commented May 22, 2024

This PR implements the feature which #1610 tried before. The purpose of this PR is essentially make users possible to directly load/unload chrono::Duration type to freely use timedelta types.

@McDic
Copy link
Contributor Author

McDic commented May 22, 2024

I encountered that the test is failing, and I found that followings are trying to write raw bytes on string from queries.

    // diesel/src/pg/types/date_and_time/chrono.rs
    #[test]
    fn some_dummy_test_function() {
        let connection = &mut connection();

        let interval_result = select(sql::<Interval>("'60 days 1 minute 123456 microseconds'"))
            .get_result::<crate::pg::data_types::PgInterval>(connection)
            .unwrap();
        println!("interval v1 result: {:?}", interval_result);
        let interval_result = select(sql::<Interval>("'2 months 1 minute 123456 microseconds'"))
            .get_result::<crate::pg::data_types::PgInterval>(connection)
            .unwrap();
        println!("interval v2 result: {:?}", interval_result);
        let interval_result = select(sql::<Interval>("'5184060 seconds 123456 microseconds'"))
            .get_result::<crate::pg::data_types::PgInterval>(connection)
            .unwrap();
        println!("interval v3 result: {:?}", interval_result);
        assert!(false);
    }

Above test will print following;

interval v1 result: PgInterval { microseconds: 3904656492434387744, days: 824208745, months: 1853191269 }
# v1 is read from [54, 48, 32, 100, 97, 121, 115, 32, 49, 32, 109, 105, 110, 117, 116, 101]
interval v2 result: PgInterval { microseconds: 3612007226513057907, days: 540090477, months: 1768846708 }
# v2 is read from [50, 32, 109, 111, 110, 116, 104, 115, 32, 49, 32, 109, 105, 110, 117, 116]
interval v3 result: PgInterval { microseconds: 3832906554667315232, days: 1936024431, months: 1852076832 }
# v3 is read from [53, 49, 56, 52, 48, 54, 48, 32, 115, 101, 99, 111, 110, 100, 115, 32]

Where these datas are produced when just reading corresponding raw string, proved by following Python code;

>>> list(b"'60 days 1 minute 123456 microseconds'")
[39, 54, 48, 32, 100, 97, 121, 115, 32, 49, 32, 109, 105, 110, 117, 116, 101, 32, 49, 50, 51, 52, 53, 54, 32, 109, 105, 99, 114, 111, 115, 101, 99, 111, 110, 100, 115, 39]
>>> list(b"'2 months 1 minute 123456 microseconds'")
[39, 50, 32, 109, 111, 110, 116, 104, 115, 32, 49, 32, 109, 105, 110, 117, 116, 101, 32, 49, 50, 51, 52, 53, 54, 32, 109, 105, 99, 114, 111, 115, 101, 99, 111, 110, 100, 115, 39]
>>> list(b"'5184060 seconds 123456 microseconds'")
[39, 53, 49, 56, 52, 48, 54, 48, 32, 115, 101, 99, 111, 110, 100, 115, 32, 49, 50, 51, 52, 53, 54, 32, 109, 105, 99, 114, 111, 115, 101, 99, 111, 110, 100, 115, 39]

What I doubt now is that there is something wrong on process of reading raw bytes data and casting it to the actual high level data structures, but not 100% sure.

@McDic
Copy link
Contributor Author

McDic commented May 22, 2024

nvm, I found I have to use "some_interval_literal"::interval as query string instead. Will fix and update the PR.

@McDic McDic marked this pull request as ready for review May 22, 2024 18:01
@weiznich weiznich requested a review from a team May 23, 2024 05:32
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 May 24, 2024
Merged via the queue into diesel-rs:master with commit 28b1148 May 24, 2024
49 checks passed
@McDic McDic deleted the feature/impl-interval branch May 24, 2024 07:25
@@ -139,6 +139,48 @@ impl FromSql<Date, Pg> for NaiveDate {
}
}

const DAYS_PER_MONTH: i32 = 30;
Copy link
Member

@Ten0 Ten0 May 25, 2024

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?

Copy link
Contributor Author

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.

Copy link
Member

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), and ToSql(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 to PgInterval and just provide functions that make conversions from that to chrono::TimeDelta reasonably easy but more explicit on that it may lose information and behave differently.

Copy link
Contributor Author

@McDic McDic May 25, 2024

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 example SELECT '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.

Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #4051 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants