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

postgres: Create PgInterval type #271

Merged
merged 2 commits into from
Jul 3, 2020
Merged

Conversation

dimtion
Copy link
Contributor

@dimtion dimtion commented Apr 19, 2020

This PR creates a new type binding for PostgreSQL INTERVAL type.

Fixes #197

Some notes:

  • The PR exposes a new type: sqlx::postgres::types::PgInterval which is fully compatible with PG INTERVAL type
  • I did not implemented to support for https://crates.io/crates/pg_interval since the project does not seems to be actively maintained.
  • I've implemented TryFrom for chrono::Duration and time::Duration, theses conversions fail if there is an overflow, if using incompatible fields (PG days and month fields) or if there is loss of precision by using nanoseconds.

@mehcode
Copy link
Member

mehcode commented Apr 19, 2020

Can you add some tests to postgres-types ?

@@ -235,6 +236,38 @@ fn postgres_epoch() -> DateTime<Utc> {
Utc.ymd(2000, 1, 1).and_hms(0, 0, 0)
}

impl TryInto<Duration> for PgInterval {
Copy link
Member

Choose a reason for hiding this comment

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

I'm reasonably sure that you want to impl TryFrom<PgInterval> for Duration instead.

}

impl PgInterval {
pub fn new(months: i32, days: i32, microseconds: i64) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

If we are making the fields of PgInterval public, there isn't much point to have the new here.

@@ -252,6 +256,40 @@ impl Encode<Postgres> for OffsetDateTime {
}
}

impl TryInto<Duration> for PgInterval {
Copy link
Member

Choose a reason for hiding this comment

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

The chrono and time files are already pretty big. This impls might be better in interval.rs with the appropriate cfg flags. Just a minor thought.

@abonander
Copy link
Collaborator

abonander commented Apr 21, 2020

theses conversions fail [...] if there is loss of precision by using nanoseconds.

This may actually cause a lot of errors if people are inserting Durations based on precise timers. Maybe instead of/in addition to a TryFrom<Duration> for PgInterval we need to have like a PgInterval::truncate_nanos(Duration) constructor or something that doesn't issue this error (but still covers the other error cases).

@dimtion
Copy link
Contributor Author

dimtion commented Apr 22, 2020

That is a good idea, I think that replacing the TryFrom behavior by silencing this type of issue could generate even more bugs that go unnoticed. So I would think that, as you said, adding a new constructor would be better. And I should add a bit of documentation to highlight the difference between the two.

I have one question however, I not that seasoned in Rust and I'm not sure how to create a function PgInterval::truncate_nanos(Duration) that could either take a chrono::Duration or a time::Duration, do you have any idea how I should do that?

Thank you

@dimtion
Copy link
Contributor Author

dimtion commented Apr 24, 2020

I've created two functions chrono_truncate_nanos and time_truncate_nanos in bd27d0e.

I'm not sure if it is the right thing to do. Please let me know how I can improve that.

@abonander
Copy link
Collaborator

@dimtion Sorry for the delay, would you mind providing impls for std::time::Duration as well? It seems fitting.

As for truncate_nanos, there's a few options I see:

  • only define the constructor for std::time::Duration and expect the user to convert their chrono::Duration or time::Duration first
  • define a trait TruncateNanos and implement it for all of [chrono, std::time, time]::Duration, then have the constructor take impl TruncateNanos
  • keep the separate constructors but add the crate name as a postfix, i.e. truncate_nanos_chrono, truncate_nanos_time, truncate_nanos_std

@jhpratt
Copy link

jhpratt commented Jun 2, 2020

I don't think this impl is a good idea. As the Interval type is a proper interval and not just a duration, attempting to treat a Duration as such is error-prone at best. In the future, the time crate will provide for a Period type that could be properly mapped to the Interval appropriately. There's no timeframe on this, but I thought it would be best to make my thoughts known before this potentially lands.

@dimtion
Copy link
Contributor Author

dimtion commented Jun 23, 2020

@jhpratt what you says makes a lot of sense, and is indeed the core of the issue I faced. An interval/period is not the same duration, and converting between the two must not be an implicit thing.

@abonander do you think creating a PR only providing the PgInterval type could be enough? This would allow to make sqlx work with PostgreSQL Interval type, something is impossible right now. All the other type compatibility are only here to be handy in the end, and could be postponed if deemed error prone.

@jhpratt
Copy link

jhpratt commented Jun 23, 2020

To be clear, I don't think converting a Period to a Duration directly will ever be allowed in the time crate. I'll almost certainly implement From<Duration> for Period when the latter is created, but only because some number of seconds would be a strict subset of what is storable in a period type.

Obviously you're free to do as you wish, but that's my perspective from the time crate.

@mehcode
Copy link
Member

mehcode commented Jun 23, 2020

I have to agree with @jhpratt here about the Period type and the concerns around conversion. As for this PR, I think we should accept a minimal useful surface for now.

  • A PgInterval type that is explicitly those three fields that fully supports the protocol and has Type + Encode + Decode.

  • Type + Encode for time::Duration, chrono::Duration, and std::time::Duration where Encode fails if we cannot express the duration as a PgInterval.

  • (in a later PR) Type + Encode + Decode for time::Period when that type is available.

@dimtion
Copy link
Contributor Author

dimtion commented Jun 28, 2020

Ok, so I've rebased this branch with master:

  • Commit 1: creates PgInterval type that fully supports Type + Encode + Decode
  • Commit 2: adds a compatibility layer for std::time::Duration, time::Duration & chrono::Duration. It implements TryFrom<Duration> for PgInterval, and fails if there is an overflow or if there is loss of precision
  • I've implemented Type + Encode for time::Duration, chrono::Duration, and std::time::Duration using this TryFrom
  • Following @abonander idea, I did not remove the truncate_nanos_* methods since they might be useful in some cases.

Let me know if this implementation is better.

@jhpratt
Copy link

jhpratt commented Jul 2, 2020

Looking at the overall diff, it looks good to me. I just checked the bits relevant to the time crate, though.

One but, I believe it would be more efficient to check if the nanos % 1000 == 0 instead of performing more complicated arithmetic.

@mehcode mehcode merged commit 4c5ea7a into launchbadge:master Jul 3, 2020
@mehcode
Copy link
Member

mehcode commented Jul 3, 2020

@dimtion Thank you ❤️ for sticking with this. I merged it and narrowed down the public API further. We can always add methods to it if it becomes a pattern people want ( the truncate approach ) but I think we're probably best off waiting for a cleaner type like time::Period.

@jhpratt Thank you as well for reviewing this as it moved along.

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.

[Postgres] Add support for INTERVAL
4 participants