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

Missing ArgAbi and BoxRet trait implementations for datums #1757

Merged
merged 5 commits into from
Jul 8, 2024
Merged

Missing ArgAbi and BoxRet trait implementations for datums #1757

merged 5 commits into from
Jul 8, 2024

Conversation

danbluhmhansen
Copy link
Contributor

@danbluhmhansen danbluhmhansen commented Jul 6, 2024

After updating to version 0.12.0-beta.0 I can no longer use JsonB as a pg_extern function argument, and I can no longer use JsonB, AnyArray, or AnyElement as state type in pg_aggregate. Unfortunately, pgrx lacked any tests for these types, so they were not checked for compilation when adding ArgAbi and RetAbi.

@eeeebbbbrrrr
Copy link
Contributor

Interesting. Surprised we don’t have tests to catch these.

Thanks @danbluhmhansen. I’ll approve CI and we’ll see about merging next week, I imagine.

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 should land with tests that prevent this omission in the future. Just simple "accepts the type, returns the type" somewhere in pgrx-tests is all.

@workingjubilee
Copy link
Member

I have a suspicion that these BoxRet impls for AnyArray and AnyElement will prove... optimistic, but if we can at least make it through a function call, then at least it kinda works.

@workingjubilee
Copy link
Member

workingjubilee commented Jul 7, 2024

Preferably, the Any* types should at least have the mercy to have their impl check that you created an AnyType with the same OID returned by get_fn_expr_rettype. Perhaps only beneath cfg(debug_assertions)? That can wait for a separate PR if you are not interested in writing it, however.

@danbluhmhansen
Copy link
Contributor Author

This should land with tests that prevent this omission in the future. Just simple "accepts the type, returns the type" somewhere in pgrx-tests is all.

Alright I have added tests that should catch possible future regressions.

  • json argument type tests to json_tests.rs
  • anyelement argument type tests to new anyelement_tests.rs
  • anynumeric argument type tests to new anynumeric_tests.rs
  • aggregate state type tests for types json, jsonb, anyarray, anyelement to aggregate_tests.rs
    • I couldn't figure out how to write an actual pg_test for the anyarray aggregate state so I left that out, besides any regression should be caught by having the pg_aggregate implementation, regardless of the tests.

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.

Thank you.

@workingjubilee workingjubilee merged commit e4fd443 into pgcentralfoundation:develop Jul 8, 2024
12 checks passed
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