-
-
Notifications
You must be signed in to change notification settings - Fork 246
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
Include type when getting argument #740
Include type when getting argument #740
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct to me but I think it could use a 1-second glance from @eeeebbbbrrrr as well before it gets merged.
EDIT: My instincts about needing a second pair of eyes were correct, this is actually too slow for usage. The code needs to special-case AnyElement specifically, but I haven't the faintest idea how.
I added an associated constant ( |
Hmm. That should work, but is it possible for us to make it only happen when AnyElement's |
I don't think it's possible to do the type lookup on demand without making Another option would be to make a separate trait method for when a type OID is needed. That approach would allow for |
a12b8bd
to
72eba3f
Compare
Hm. Should we use |
|
...yeaaaah, that's not going to work, then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreeing with my naming sense is not required, but I think makes it clearer why this function should exist.
Co-authored-by: Jubilee <46493976+workingjubilee@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should find a cleaner solution in the future but I think this will do for right this second: it more or less brings us full circle to where we were before, but with at least a direction to go this time.
Can we get rid of the |
@eeeebbbbrrrr I think this PR fixed all the cases where |
547: Upgrade to pgx 0.5.0 r=Smittyvb a=Smittyvb Upgrades to pgx `0.5.0-beta.1` (0.5.0 will hopefully be released in 0-7 days). I originally tested on another branch with a lot of code commented out; this branch cherry picks the commits that fix errors but not the ones that comment code out. To do: - [x] Fix the three failing tests in `datum_utils.rs` (pgcentralfoundation/pgrx#740) - [x] Remove workaround for lack of `Copy` on some types - [x] Make update tester work in CI (#552) - [x] Change pgx version from 0.5.0-beta.1 to 0.5.0 - [ ] Publish new CI docker image with pgx 0.5.0 right before merging (#571) Future work: - Replace `use pgx::*` with `use pgx::prelude::*` - Use native pgx types instead of `crate::raw` - Use `#[pg_aggregate]` instead of our own aggregate builder macro Co-authored-by: Smitty <smitty@timescale.com>
This fixes #735 by passing the type of an argument when getting an argument.