feat: bootstrap jiff-sqlx development #141
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ use sqlx::encode::IsNull; | |
| use sqlx::error::BoxDynError; | ||
| use sqlx::postgres::types::{Oid, PgInterval}; | ||
| use sqlx::postgres::{PgArgumentBuffer, PgHasArrayType, PgTypeInfo}; | ||
| use sqlx::{Database, Decode, Encode, Postgres, Type}; | ||
|
|
||
| impl Type<Postgres> for SignedDuration { | ||
| fn type_info() -> PgTypeInfo { | ||
|
|
@@ -59,6 +59,29 @@ impl Encode<'_, Postgres> for SignedDuration { | |
| } | ||
| } | ||
|
|
||
| impl<'r> Decode<'r, Postgres> for SignedDuration { | ||
| fn decode( | ||
| value: <Postgres as Database>::ValueRef<'r>, | ||
| ) -> Result<Self, BoxDynError> { | ||
| let pg_interval = PgInterval::decode(value)?; | ||
|
|
||
| if pg_interval.months != 0 { | ||
| return Err( | ||
| "Cannot convert months in `INTERVAL` to SignedDuration".into(), | ||
| ); | ||
| } | ||
|
|
||
| if pg_interval.days != 0 { | ||
| return Err( | ||
| "Cannot convert days in `INTERVAL` to SignedDuration".into() | ||
| ); | ||
| } | ||
|
|
||
| let micros = pg_interval.microseconds; | ||
|
There was a problem hiding this comment. @maxcountryman Ah so I think this is where there is a correctness issue? The non-microseconds units are being silently ignored here. There was a problem hiding this comment. This is almost certainly incorrect, yes. However, I'm also pointing out that Postgres can be configured to output ISO 8601. Your docs say SignedDuration is not capable of parsing ISO 8601 with non-zero days. Moreover, my impression is that Spans may be preferred for ISO 8601. Regardless, Postgres will use non-zero days when configured this way so that must be handled somehow. In my own programs I just use Span. I said as much earlier in the thread but the original author did not respond after I pointed this out. I don't think an implementation that doesn't address this somehow should be published personally. There was a problem hiding this comment. Just to confirm there is a correctness issue here, this implementation omits two fields, There was a problem hiding this comment. Okay thanks, I understand now. I agree. I'll take a closer look later. There was a problem hiding this comment. @maxcountryman Thanks for your feedback! Your analysis is correct. The decode part is added three days ago at 2f17955 while I'd like to use SignedDuration to hold PG's interval. The background in my use case is that all the interval is written by my code and thus it's guaranteed only microseconds unit are filled. But for general purpose usage, this may not be true. There is a related discussion at #174 (reply in thread). In short, I'd first remove the decode impl for SignedDuration now because you can hardly convert days & months to seconds without a relative datetime by jiff's design. But perhaps we can support codec over However, the encode part of SignedDuration should be fine. There was a problem hiding this comment. Or instead, we may check months and days are 0 when decode to |
||
| Ok(SignedDuration(jiff::SignedDuration::from_micros(micros))) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use crate::ToSignedDuration; | ||
|
|
||
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.
@maxcountryman I'd re-add the impl for decoding SignedDuration.
I saw your reaction on the comment but unsure what the concrete issue you found. Maybe you can reply here.
For support of Span and convert between Span and PgInterval for sqlx codec, I'd leave it to the next PR if we get progress here, or there is a real usage requirement.