Support for (Pg)Interval with chrono::Duration

[Rudi3]

Preparation for https://github.com/SeaQL/sea-orm/issues/426

Support for

• [x] std::time::Duration
• [x] chrono::Duration

I'm not sure if it makes sense to use std::time::Duration without chrono

postgres-types doesn't seem to support ToSql for chrono::Duration - at least it wasn't listed in the docs. So I'm not sure if this is a big deal in postgres.rs

sqlx will panic if a conversion fails; if:

• Duration is longer than what PgInterval can store
• Duration contains nanoseconds - too small for PgInterval

[ikrivosheev]

@Rudi3 hello, are there any plans to finish?)

[Rudi3]

@ikrivosheev hello, i have to admit that I forgot about it. But this was pretty much done except for the uncertainty about ToSql. Do you think there might be any other problems?

[ikrivosheev]

@Rudi3 which uncertainty with ToSql? I dont understand

[Rudi3]

@ikrivosheev src/driver/postgres.rs I decided to throw unimplemented if to_sql is called in the postgres driver.rs because postgres-types doesn't implement ToSql for chrono::Duration - as far as I could tell. I'm not sure if that would lead to any problems. I'll have to check if support was added in the meantime, however. At a glance, it doesn't seem like it: https://docs.rs/postgres-types/latest/postgres_types/trait.ToSql.html If this is needed, it might block this for now.

[ikrivosheev]

Ah... I understood! We can create issue in postgres-types and wait for implantation or implement it yourself.

I will create issue.

[ikrivosheev]

Found: https://github.com/sfackler/rust-postgres/issues/60

[billy1624]

Hey @Rudi3, welcome back!

After second thought, I think we can support time::Duration & chrono::Duration. Just like what we did to all other datetime values.

• https://github.com/SeaQL/sea-query/blob/70cd3dcf4ed26334cc349c1ba419e3f22c5f66d8/src/value.rs#L51-L53
• https://github.com/SeaQL/sea-query/blob/70cd3dcf4ed26334cc349c1ba419e3f22c5f66d8/src/value.rs#L75-L77

[Rudi3]

Hey, @billy1624! Unfortunately ToSql isn't implemented for time::Duration in postgres-types either. But I'll see if I can add time::Duration at least in here when I get to it.

[etsea117]

@Rudi3 any update on this?

[billy1624]

Hey @Rudi3, I'm sorry for the delay.

Hey, @billy1624! Unfortunately ToSql isn't implemented for time::Duration in postgres-types either. But I'll see if I can add time::Duration at least in here when I get to it.

I think postgres driver implements non of chrono::Duration, time::Duration and std::time::Duration?

We could get this PR merge before moving on to https://github.com/SeaQL/sea-orm/pull/456

[Rudi3]

Hello, I revised this PR and noticed that to_sql seems be run on a Json value rather than on e.g. chrono::DateTime or chrono::Duration. So it's converted to a string in a previous step. If I got that right, we don't actually need the ToSql Trait for chrono::Duration or time::Duration - but just a conversion to a precise string that is accepted by postgres.

For now, I opted for the ISO8601 duration format that's provided by chrono::Duration. Negative durations are expressed by a minus before the statement like -P3DT4H5M6S. But postgres docs mention that each field should be negative to avoid misinterpretation.

So maybe I should rewrite the formatter to do exactly this. Furthermore, the maximum allowed precision is 6. So should any other digits be rounded/cut off/etc? Also, maybe it's possible to optimize the format to just provide total seconds with up to 6 decimal places - if ISO8601 and postgres allow for this. This would also dodge the multiple field issue related to the sign. Requesting some feedback on this and maybe confirmation of my assumption about the to_sql conversion, please. Thank you!

Edit: The code in src/backend/query_builder.rs doesn't seem to be called during my tests with time::Duration. I can read and write values without reaching this code - so I'm not sure what it's for and how it should behave.

sea-query-binder/src/sqlx_postgres.rs seems to be doing all the work when writing but I'm not sure if it's sufficient - as the duration is just passed over as far as I can see.

Currently, it appears to be working with time:Duration. chrono:Duration isn't tested yet. Due to the aforementioned problems, I'm not sure if it's safe.

[DevSlashRichie]

Is there any advancement on this? Is there anything I can help with?