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

New sql type mapping #615

Merged
merged 61 commits into from
Sep 20, 2022
Merged

New sql type mapping #615

merged 61 commits into from
Sep 20, 2022

Conversation

Hoverbear
Copy link
Contributor

@Hoverbear Hoverbear commented Aug 2, 2022

This revamps the SQL type mapping system to be based around the trait SqlTranslatable instead of the current TypeId based system.

Releasing this has breaking changes and requires a 0.5.0

What's changed

  • pgx is now able to map Rust types to SQL based on the existence of an SqlTranslatable trait (and its results). This trait is implemented for all types pgx supports, and implementations are generated in #[derive(PostgresType)] and #[derive(PostgresEnum)] macros for users.
  • Foundation added to handle const generics for tasks such as supporting NUMERIC(1, 10).
  • Foundation added for generic types for tasks such as supporting users returning Result<T,E> once codegen is updated.
  • The pgx-utils/src/rewriter.rs itemfn code was mostly migrated into the sql_entity_graph code and integrated.
  • Source-only SQL mapping still exists and happens, however @workingjubilee and I discussed that there was a way to get this working with a newtype pattern. This was left for the future.
  • SQL generation is, in general, quite a bit more strict about how it works, and users should see more, earlier build time errors when they try to do things that won't work.

Breaking changes

  • We no longer maintain a big hashmap of "Rust type -> SQL type" for builtin types like i32 and char which is passed around during the SQL generation process. The pgx_graph_magic!() (and thus pgx_module_magic!()) macros have had their ABIs changed. Old version of cargo-pgx won't work anymore.
  • It is no longer possible to return impl Iterator<Item = ...> from #[pg_extern]s. Instead, we have SetOfIterator<'a, T> and TableIterator<'a, T> which generate SETOF T and TABLE T in SQL generation respectively.
    • Users can transform set returning functions from:
      fn fluffy() -> impl Iterator<Item = i32> {
         vec![1,2,3].into_iter()
      }
      Into:
      fn fluffy() -> SetOfIterator<'static, i32> {
         SetOfIterator::new(vec![1,2,3].into_iter())
      }
    • Users can transform table returning functions from:
      fn fluffy() -> impl Iterator<Item = (named!(name, String), named!(i32),)> {
         vec![1,2,3].into_iter()
      }
      Into:
      fn fluffy() -> TableIterator<'static, (named!(name, String), named!(i32),)> {
         TableIterator::new(vec![1,2,3].into_iter())
      }
    • If your function accepts some borrows with lifetimes, it may need to use those lifetimes instead of 'static.

Signed-off-by: Ana Hobden <[email protected]>

Function inference works too

Signed-off-by: Ana Hobden <[email protected]>

Restructure a bit

Signed-off-by: Ana Hobden <[email protected]>

Strings and Numeric work, srf somewhat

Signed-off-by: Ana Hobden <[email protected]>
Signed-off-by: Ana Hobden <[email protected]>
Signed-off-by: Ana Hobden <[email protected]>
Signed-off-by: Ana Hobden <[email protected]>
Signed-off-by: Ana Hobden <[email protected]>
Signed-off-by: Ana Hobden <[email protected]>
Signed-off-by: Ana Hobden <[email protected]>
Signed-off-by: Ana Hobden <[email protected]>
Signed-off-by: Ana Hobden <[email protected]>
Signed-off-by: Ana Hobden <[email protected]>
Signed-off-by: Ana Hobden <[email protected]>
@Hoverbear Hoverbear self-assigned this Aug 2, 2022
They aren't necessary and are mostly due to a commit
that introduced those bounds for certain operations,
but that was not necessary then in those circumstances either.
@workingjubilee
Copy link
Member

workingjubilee commented Sep 19, 2022

@Hoverbear Alright. I now have this building both ZomboDB and <REDACTED>, and the latter's tests even all pass. Less sure about ZomboDB, some do, some probably weren't set up correctly in my env, but also that has a slower release cadence and so we can probably defer the ZomboDB release if we need to fix bugs.

@workingjubilee
Copy link
Member

This PR will now also fix #615.

@workingjubilee
Copy link
Member

Apparently this now works for ZomboDB on build environments that aren't my local? I have no idea what's going wrong on my local. I can fix it later if necessary.

@workingjubilee
Copy link
Member

Okay, so there are a few lingering test errors at zombodb/zombodb#763 but

[3:02 PM] Hoverbear: I want to look at the timestamp thing
[3:04 PM] Hoverbear: Hmmm yes it looks fine
[3:04 PM] Hoverbear: I think it's just a serde thing
[3:04 PM] Hoverbear: Okay I feel comfortable-ish with the pgx new-type-mapping pr merging
[3:05 PM] Hoverbear: If you wanna, go ahead ❤️

These errors were likely introduced by #643 which otherwise we expect to have significant performance/correctness improvements from, so we are not reverting that (but we might be adjusting the serialization slightly). Otherwise we're good, it looks like.

@Hoverbear
Copy link
Contributor Author

Yay!

@Hoverbear
Copy link
Contributor Author

We noticed this seems to have missed some dead code at https://github.com/tcdi/pgx/blob/cafefaaf8810143059f7e731e6cd464a38499c6a/pgx/src/lib.rs#L156

@workingjubilee
Copy link
Member

Opened #697

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.

2 participants