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

add support for array arguments and return types in UDFs #454

Merged
merged 18 commits into from
Nov 30, 2023

Conversation

imor
Copy link
Contributor

@imor imor commented Nov 22, 2023

What kind of change does this PR introduce?

Feature

What is the current behavior?

Arrays are not supported in arguments and as return types of user defined functions.

What is the new behavior?

Arrays are supported in arguments and as return types of user defined functions.

Additional context

Default values for array type arguments are not supported.

@imor imor changed the title Add array support add support for array arguments and return types in UDFs Nov 23, 2023
@imor imor marked this pull request as ready for review November 23, 2023 06:14
@olirice olirice self-requested a review November 27, 2023 13:58
Copy link
Contributor

@olirice olirice left a comment

Choose a reason for hiding this comment

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

nice!

as written there's nothing to exclude returning arrays of complex types but they aren't yet fully supported. Example below:

Screenshot 2023-11-27 at 2 18 55 PM

Either:

  • updating to support arrays of row types
  • filtering out functions returning non-scalar type arrays

would be great

We could also use an entry in the changelog and an update to documentation for this change

@imor
Copy link
Contributor Author

imor commented Nov 29, 2023

@olirice arrays of composite types are filtered out now. Docs and changelog have also been updated.

Copy link
Contributor

@olirice olirice left a comment

Choose a reason for hiding this comment

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

I just noticed the logic for applying enum variant remapping is in ColumnBuilder so those re-mapping will not take effect if a function returns a scalar enum or an array of enum.

I've approved, but please either:

  • filter those function types out
  • refactor the logic so it applies to function returns too

everything else you've got here looks great!

@imor
Copy link
Contributor Author

imor commented Nov 30, 2023

Filtered out functions with enum and enum arrays as args or return values. Added a task in #410 to add support for them later.

@imor imor merged commit 6a73cf7 into master Nov 30, 2023
4 checks passed
@imor imor deleted the add_array_support branch November 30, 2023 10:40
@dvv
Copy link

dvv commented Nov 30, 2023

@imor Please verify this is not my setup and help to get rid of the error:

begin;

create schema if not exists a1; grant usage on schema a1 to public;
create table a1.foo(id int primary key); grant select on table a1.foo to public;
insert into a1.foo (id) values (1);
create or replace function a1.bar(a1.foo) returns int[] stable return array[1, 2, 3]::int[];
grant execute on function a1.bar(a1.foo) to public;
set local search_path to a1;
comment on schema a1 is e'@graphql({"inflect_names": true})';
select graphql.resolve($$query {fooCollection {edges{node{ id bar }}}}$$); -- invalid return type from function

rollback;

@dvv
Copy link

dvv commented Nov 30, 2023

@imor I believe __Type::List(_) => FunctionSelection::ScalarSelf, should be appended here https://github.com/supabase/pg_graphql/blob/master/src/builder.rs#L1585

@dvv dvv mentioned this pull request Dec 1, 2023
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