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

Opaque Date type with +/- infinity support #622

Merged
merged 6 commits into from
Aug 16, 2022

Conversation

mhov
Copy link
Contributor

@mhov mhov commented Aug 12, 2022

I switched the Date type with one that's an alias for the i32 datum. try_get_date(&self) will get you a time:Date if it's within the time crate's bounds, or an Err(pg_epoch_days: i32) if out of range. -infinity and infinity are handled on their own.

I moved the serialization #[test] to #[pg_test] so that serialization can leverage PG's built in serialization. This is especially important for values outside of time:Date bounds which we couldn't format/serialize otherwise.

@mhov mhov changed the title Opaque type style Date type with +/- infinity support Opaque Date type with +/- infinity support Aug 12, 2022
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

This looks good! I mostly have a few remarks about the question of the unsafe code / PGX <-> Postgres interop.

pgx/src/datum/date.rs Show resolved Hide resolved
pgx/src/datum/date.rs Outdated Show resolved Hide resolved
pgx/src/datum/date.rs Outdated Show resolved Hide resolved
pgx/src/datum/date.rs Show resolved Hide resolved
pgx/src/datum/date.rs Show resolved Hide resolved
pgx/src/datum/date.rs Outdated Show resolved Hide resolved
@workingjubilee workingjubilee self-requested a review August 15, 2022 22:48
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

I think this looks good!

My only remarks are, now that the dust has settled on everything else and I can see the conversions we are going to provide: When the conversion is for a Rust type and not a C typedef, we can use the conversion traits that Rust normally provides via the prelude to define the conversion.

There are reasons to do otherwise, like type inference on the conversion and syntactic niceties, but I don't think they apply here because it's a fairly direct conversion. Rather, the pressure is on to provide it ourselves, since only two other crates can implement these conversions (and in practice only one).

pgx/src/datum/date.rs Outdated Show resolved Hide resolved
pgx/src/datum/date.rs Outdated Show resolved Hide resolved
@workingjubilee
Copy link
Member

I think by returning the deprecated fn new to existence, this changeset has minimized the blast radius, and... I really, really hope no one was actually implicitly using methods via time::Date. If they did... well, it should only be a quick time::Date::from(date)/date.into() away. 🙈

Hopefully they will forgive us when we say it's ultimately for performance reasons.

@Hoverbear
Copy link
Contributor

I think it's reasonable. :)

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

Successfully merging this pull request may close these issues.

3 participants