feat(pgrx): add chrono feature for easy conversions #1603
Conversation
|
I think you're probably on the right track here. I suppose we'd want to also have conversions from the I don't know how far @workingjubilee might want to take this in terms of also supporting |
|
Yeah, this seems like the right direction. I don't think we want to support IntoDatum/FromDatum for these types. I would rather move towards making it easier to distinguish when something is a "zero-overhead" or "direct" conversion that is natively supported by Postgres, and when something requires additional logic like these types always will. |
|
Thanks for the check @workingjubilee @eeeebbbbrrrr and for the heads up -- just to confirm I do plan to add the conversions both ways. Will finish up the rest of it in this pattern and turn this to ready for review hopefully soon! |
t3hmrman
force-pushed
the
feat(pgrx)=add-chrono-feature
branch
from
0060346
to
602e592
Compare
t3hmrman
force-pushed
the
feat(pgrx)=add-chrono-feature
branch
3 times, most recently
from
f82cda2
to
d389f30
Compare
|
Hey @workingjubilee @eeeebbbbrrrr just finished the first version of this, would love a review whenever you can find some time! I've tried to be pretty explicit about where the microseconds-only limitation of pg comes into play. I'm also equally happy to put this functionality in some sort of external crate if you'd prefer to not upstream this functionality as well! |
|
Hi ! Is there's a way for me to help moving this PR forward ? It would be very useful in my own extension... |
t3hmrman
force-pushed
the
feat(pgrx)=add-chrono-feature
branch
from
d389f30
to
ba845a4
Compare
|
Rebased just to make sure that's not blocking progress here :) |
t3hmrman
force-pushed
the
feat(pgrx)=add-chrono-feature
branch
from
ba845a4
to
77a2824
Compare
|
Sorry, all my focus was being consumed by #1731 since that was one of our last release blockers so reviewing anything that wasn't incredibly trivial was backburnered. |
| fn chrono_simple_date_conversion() -> DtcResult<()> { | ||
| let original = pgrx::Date::new(1970, 1, 1)?; | ||
| let d = chrono::NaiveDate::try_from(original)?; | ||
| assert_eq!(d.year(), original.year(), "year matches"); | ||
| assert_eq!(d.month(), 1, "month matches"); | ||
| assert_eq!(d.day(), 1, "day matches"); | ||
| let backwards = pgrx::Date::try_from(d)?; | ||
| assert_eq!(backwards, original); | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Ensure simple conversion ([`pgrx::Time`] -> [`chrono::NaiveTime`]) works | ||
| #[pg_test] | ||
| fn chrono_simple_time_conversion() -> DtcResult<()> { | ||
| let original = pgrx::Time::new(12, 1, 59.0000001)?; | ||
| let d = chrono::NaiveTime::try_from(original)?; | ||
| assert_eq!(d.hour(), 12, "hours match"); | ||
| assert_eq!(d.minute(), 1, "minutes match"); | ||
| assert_eq!(d.second(), 59, "seconds match"); | ||
| assert_eq!(d.nanosecond(), 0, "nanoseconds are zero (pg only supports microseconds)"); | ||
| let backwards = pgrx::Time::try_from(d)?; | ||
| assert_eq!(backwards, original); | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Ensure simple conversion ([`pgrx::Timestamp`] -> [`chrono::NaiveDateTime`]) works | ||
| #[pg_test] | ||
| fn chrono_simple_timestamp_conversion() -> DtcResult<()> { | ||
| let original = pgrx::Timestamp::new(1970, 1, 1, 1, 1, 1.0)?; | ||
| let d = chrono::NaiveDateTime::try_from(original)?; | ||
| assert_eq!(d.hour(), 1, "hours match"); | ||
| assert_eq!(d.minute(), 1, "minutes match"); | ||
| assert_eq!(d.second(), 1, "seconds match"); | ||
| assert_eq!(d.nanosecond(), 0, "nanoseconds are zero (pg only supports microseconds)"); | ||
| let backwards = pgrx::Timestamp::try_from(d)?; | ||
| assert_eq!(backwards, original, "NaiveDateTime -> Timestamp return conversion failed"); | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Ensure simple conversion ([`pgrx::TimestampWithTimeZone`] -> [`chrono::DateTime<Utc>`]) works | ||
| #[pg_test] | ||
| fn chrono_simple_datetime_with_time_zone_conversion() -> DtcResult<()> { | ||
| let original = pgrx::TimestampWithTimeZone::with_timezone(1970, 1, 1, 1, 1, 1.0, "utc")?; | ||
| let d = chrono::DateTime::<Utc>::try_from(original)?; | ||
| assert_eq!(d.hour(), 1, "hours match"); | ||
| assert_eq!(d.minute(), 1, "minutes match"); | ||
| assert_eq!(d.second(), 1, "seconds match"); | ||
| assert_eq!(d.nanosecond(), 0, "nanoseconds are zero (pg only supports microseconds)"); | ||
| let backwards = pgrx::TimestampWithTimeZone::try_from(d)?; | ||
| assert_eq!(backwards, original); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Can you, in addition to these simpler tests, set up a proptest like in https://github.com/pgcentralfoundation/pgrx/blob/77a28242fd89372a6fd7cc4ded1fcae9a63319f8/pgrx-tests/src/tests/proptests.rs so that these conversions are fairly invariant?
It doesn't have to roundtrip stuff through SPI, since that's not really what we'd be testing, so it doesn't need to use the exact pattern I used there. It can do something simpler with just handling https://github.com/pgcentralfoundation/pgrx/blob/77a28242fd89372a6fd7cc4ded1fcae9a63319f8/pgrx-tests/src/proptest.rs
There was a problem hiding this comment.
Absolutely -- yeah I didn't think the roundtripping was necessary, but a prop test would definitely make a ton of sense here.
- Add property test for conversions
|
@workingjubilee absolutely no problem -- thanks for taking a look (P.S. I saw the other thing you were working on 👍 👍 👍). Will add the additional tests! |
This commit adds a `chrono` feature flag to `pgrx`, which enables conversions between `pgrx` native date/time (with or without timezone) and `chrono` types like `NaiveDateTime`.
t3hmrman
force-pushed
the
feat(pgrx)=add-chrono-feature
branch
from
77a2824
to
d2f1002
Compare
Signed-off-by: vados <vados@vadosware.io>
This commit adds a
chronofeature flag topgrx, which enables conversions betweenpgrxnative date/time (with or without timezone) andchronotypes likeNaiveDateTime.