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

feat: support jiff integration for postgresql #3511

Closed
wants to merge 7 commits into from

Conversation

tisonkun
Copy link

This refers to #3487.

My use case is for PostgreSQL + jiff only. So, I start with this code path. Hopefully, we don't need to support all backends at once but gradually make it with multiple contributions.

cc @abonander I'd like to know how we can add such an experimental feature flag and perhaps with partial support.

cc @BurntSushi if you have some time to review the use of jiff is correct & optimal.

sqlx-core/src/types/mod.rs Outdated Show resolved Hide resolved
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Ok(match value.format() {
PgValueFormat::Binary => {
let naive = <DateTime as Decode<Postgres>>::decode(value)?;
naive.to_zoned(TimeZone::UTC)?.timestamp()
Copy link
Author

Choose a reason for hiding this comment

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

@BurntSushi Is this the proper way to convert DateTime to Timestamp? I don't find a direct way though.

Copy link

@BurntSushi BurntSushi Sep 19, 2024

Choose a reason for hiding this comment

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

There's no direct way because there is no such thing as a general way of converting a civil datetime to a timestamp. The former is naive or civil or local time, where as the latter is a precise instant in time. The only way to convert the former to the latter is with an offset. In this particular case, you've chose an offset of 0 or UTC. Whether that's correct or not depends on the meaning of naive from PostgreSQL.

I think you have things backwards here. postgres_epoch_datetime should probably return a Timestamp. And then you should use that to implement Decode<'r, Postgres> for Timestamp, and not involve civil times at all.

Then have a different function that returns the base for civil time.

See https://github.com/sfackler/rust-postgres/pull/1164/files for how it's done there. You should be able to copy that same conceptual approach.

Copy link
Author

Choose a reason for hiding this comment

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

Yes correct. Thank you.

Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
Signed-off-by: tison <[email protected]>
@abonander
Copy link
Collaborator

The disposition has not changed in the past two weeks: I do not want to land a library that's still in its 0.1 release cycle as part of our public API. It's just too significant of a SemVer hazard.

There's nothing stopping you from creating newtype wrappers for jiff types and implementing the required traits in your own crate. That's a well known workaround for the orphan rule.

@tisonkun
Copy link
Author

There's nothing stopping you from creating newtype wrappers for jiff types and implementing the required traits in your own crate. That's a well known workaround for the orphan rule.

Yes. I'm planning do this before sqlx support jiff at upstream.

@tisonkun
Copy link
Author

@abonander PgTypeInfo's constructor and constants are private to prevent it implemented outside properly?

image

@abonander
Copy link
Collaborator

You can construct it from a type name or OID:

OID would be better in this case since OIDs are stable for built-in types, and it'll save the connection from having to look it up by name.

You can find the OID for a type by querying pg_catalog.pg_type or looking it up in this file: https://github.com/postgres/postgres/blob/master/src/include/catalog/pg_type.dat

@tisonkun
Copy link
Author

@abonander thank you. I saw with_oid returns a PgType::DeclareWithOid inside instead of PgType::Timestamptz and not sure if it's for custom type only.

We may move to #3512 for this discussion and if there is no blocker to expose these consts, perhaps we can do it.

For oid, it's said that "the OID for a type is very dependent on the environment."

@tisonkun
Copy link
Author

Closed for now as no progress can be made and we can always go back here as a reference.

@tisonkun tisonkun closed this Sep 20, 2024
@tisonkun tisonkun deleted the jiff-integration branch September 20, 2024 02:16
impl Encode<'_, Postgres> for DateTime {
fn encode_by_ref(&self, buf: &mut PgArgumentBuffer) -> Result<IsNull, BoxDynError> {
// TIMESTAMP is encoded as the microseconds since the epoch
let micros = (*self - postgres_epoch_datetime()).get_microseconds();
Copy link
Author

Choose a reason for hiding this comment

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

Incorrect arith, should probably use:

        let micros = self
            .0
            .duration_since(postgres_epoch_timestamp())
            .as_micros();
        if micros > i64::MAX as i128 || micros < i64::MIN as i128 {
            return Err(format!("Timestamp {} out of range for Postgres: {micros}", self.0).into());
        }
        Encode::<Postgres>::encode(micros as i64, buf)

@abonander
Copy link
Collaborator

if there is no blocker to expose these consts, perhaps we can do it.

There is: I don't want the docs page for PgTypeInfo to be a miles-long list of associated consts with its methods and trait impls buried at the very bottom.

Perhaps these could live in a postgres::types::catalog module, but I'd need to think on that. I'd want to do all of them at once, which is out-of-scope for this PR.

Generally, for creating newtype wrappers you can just invoke the Type::type_info() method of an existing supported type, but here that would mean depending on either chrono or time as well.

For oid, it's said that "the OID for a type is very dependent on the environment."

Literally the next sentence in those docs:

If you only ever use one database or if this is an unhandled built-in type, you should be fine.

The wording could be better, admittedly, but these are built-in types we're talking about.

@tisonkun
Copy link
Author

Generally, for creating newtype wrappers you can just invoke the Type::type_info() method of an existing supported type, but here that would mean depending on either chrono or time as well.

Yes. The purpose I'd make a jiff wrapper is to avoid dependency to chrono or time but use jiff everywhere.

Perhaps these could live in a postgres::types::catalog module, but I'd need to think on that. I'd want to do all of them at once, which is out-of-scope for this PR.

Sounds reasonable. The Oid workaround works. But I'd prefer a constant instead of magic literal. Types/Consts staticly check the correctness ..

The wording could be better, admittedly, but these are built-in types we're talking about.

Thank you. I make it work now:

pub struct Timestamp(pub jiff::Timestamp);

impl Type<Postgres> for Timestamp {
    fn type_info() -> PgTypeInfo {
        // 1184 => PgType::Timestamptz
        PgTypeInfo::with_oid(Oid(1184))
    }
}

impl PgHasArrayType for Timestamp {
    fn array_type_info() -> PgTypeInfo {
        // 1185 => PgType::TimestamptzArray
        PgTypeInfo::with_oid(Oid(1185))
    }
}

impl Encode<'_, Postgres> for Timestamp {
    fn encode_by_ref(&self, buf: &mut PgArgumentBuffer) -> Result<IsNull, BoxDynError> {
        // TIMESTAMP is encoded as the microseconds since the epoch
        let micros = self
            .0
            .duration_since(postgres_epoch_timestamp())
            .as_micros();
        if micros > i64::MAX as i128 || micros < i64::MIN as i128 {
            return Err(format!("Timestamp {} out of range for Postgres: {micros}", self.0).into());
        }
        Encode::<Postgres>::encode(micros as i64, buf)
    }

    fn size_hint(&self) -> usize {
        size_of::<i64>()
    }
}

impl<'r> Decode<'r, Postgres> for Timestamp {
    fn decode(value: <Postgres as Database>::ValueRef<'r>) -> Result<Self, BoxDynError> {
        Ok(match value.format() {
            PgValueFormat::Binary => {
                // TIMESTAMP is encoded as the microseconds since the epoch
                let us = Decode::<Postgres>::decode(value)?;
                let ts = postgres_epoch_timestamp().checked_add(SignedDuration::from_micros(us))?;
                Timestamp(ts)
            }
            PgValueFormat::Text => {
                let s = value.as_str()?;
                let ts = jiff::Timestamp::from_str(s)?;
                Timestamp(ts)
            }
        })
    }
}

fn postgres_epoch_timestamp() -> jiff::Timestamp {
    jiff::Timestamp::from_str("2000-01-01T00:00:00Z")
        .expect("2000-01-01T00:00:00Z is a valid timestamp")
}

My major scenarios is converting timestamp(tz) in postgres.

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.

3 participants