Adding support for filtering on array column types #520

mlabisi · 2024-05-15T09:09:37Z

What kind of change does this PR introduce?

This PR adds the ability to filter by array columns and resolves #460

What is the current behavior?

Array columns are not filterable, and the schema does not create {Scalar}ListFilter types.

What is the new behavior?

Array columns are filterable, and the schema now includes {Scalar}ListFilter types!

Additional context

Note that {Scalar}ListFilter was not created for Scalar::Opaque!

olirice

thanks for opening

could we scope this down to just filtering on contains and contained by?

I'm not sold on overlap or ordering so it would be great if we could discuss/handle those parts separately

mlabisi · 2024-05-15T17:02:39Z

thanks for opening

could we scope this down to just filtering on contains and contained by?

I'm not sold on overlap or ordering so it would be great if we could discuss/handle those parts separately

@olirice Thanks for taking a look!

I think removing the ordering changes from this PR makes sense 💯 but I do want to make a case for the overlap filter as...

...it's a built-in, native Postgres operator that's also supported by PostgREST (I don't see how exposing built-in operators can hurt!)
...selfishly, I have a use case for it 😅 (checking if an array of strings overlap)

olirice · 2024-05-15T18:53:47Z

I have a use case for it 😅 (checking if an array of strings overlap)

could you expand on the specifics for this pls?

Two of my arguments against are:

Its an unusual need (hoping your example my clarify this one)
you can accomplish the same thing with combinations of and/or/contains/contained_by

mlabisi · 2024-05-15T19:16:43Z

I have a use case for it 😅 (checking if an array of strings overlap)

could you expand on the specifics for this pls?

Two of my arguments against are:

Its an unusual need (hoping your example my clarify this one)

you can accomplish the same thing with combinations of and/or/contains/contained_by

@olirice It may be an unusual need, but I hope that doesn't make it any less valid 😅

My specific use case is pretty domain-specific, but I can use the existing schema in the seed.sql file to give a general example that has the same idea!

Given the following table

create table blog_post(
    id uuid not null default gen_random_uuid() primary key,
    blog_id integer not null references blog(id) on delete cascade,
    title varchar(255) not null,
    body varchar(10000),
    tags TEXT[],
    status blog_post_status not null,
    created_at timestamp not null
);

and the following data

insert into blog_post (blog_id, title, body, tags, status, created_at)
values
    ((SELECT id FROM blog WHERE name = 'A: Blog 1'), 'Post 1 in A Blog 1', 'Content for post 1 in A Blog 1', '{"tech", "update"}', 'RELEASED', NOW()),
    ((SELECT id FROM blog WHERE name = 'A: Blog 1'), 'Post 2 in A Blog 1', 'Content for post 2 in A Blog 1', '{"announcement", "tech"}', 'PENDING', NOW()),
    ((SELECT id FROM blog WHERE name = 'A: Blog 2'), 'Post 1 in A Blog 2', 'Content for post 1 in A Blog 2', '{"personal"}', 'RELEASED', NOW()),
    ((SELECT id FROM blog WHERE name = 'A: Blog 2'), 'Post 2 in A Blog 2', 'Content for post 2 in A Blog 2', '{"update"}', 'ARCHIVED', NOW()),
    ((SELECT id FROM blog WHERE name = 'A: Blog 3'), 'Post 1 in A Blog 3', 'Content for post 1 in A Blog 3', '{"travel", "adventure"}', 'PENDING', NOW()),
    ((SELECT id FROM blog WHERE name = 'B: Blog 3'), 'Post 1 in B Blog 3', 'Content for post 1 in B Blog 3', '{"tech", "review"}', 'RELEASED', NOW()),
    ((SELECT id FROM blog WHERE name = 'B: Blog 3'), 'Post 2 in B Blog 3', 'Content for post 2 in B Blog 3', '{"coding", "tutorial"}', 'PENDING', NOW());

I'd like to be able to execute equivalent of the following valid SQL statement:

SELECT bp.id
FROM blog_post bp
WHERE array["tech", "update"] && bp.tags

Sure, it'd be possible to split that where clause into multiple contains statements, but && is a totally valid operation that's there for the taking. In my opinion, exposing it as a filter is straightforward and can simplify queries for cases like mine. I personally don't see the harm in exposing it as a filter—but that's just me 😄

mlabisi · 2024-05-17T00:28:46Z

dockerfiles/db/setup.sql

@@ -58,6 +58,7 @@ create table blog_post(
    blog_id integer not null references blog(id) on delete cascade,
    title varchar(255) not null,
    body varchar(10000),
+    tags TEXT[],


This column was added to allow live validation beyond just the regression tests for filtering by array columns!

src/graphql.rs

mlabisi · 2024-05-17T00:38:38Z

src/graphql.rs

@@ -3541,6 +3555,8 @@ impl ___Type for FilterTypeType {
                            default_value: None,
                            sql_type: None,
                        },
+                        // shouldn't happen since we've covered all cases in supported_ops
+                        _ => panic!("encountered unknown FilterOp")


The Scalar's supported_ops won't contain any of the array-only filter operations, so we're safe to ignore them here.

Maybe we should restructure the FilterOp and related types so that we can avoid this catchall panic. As this PR is already long enough, I'm ok with doing that as a separate PR. Just open a ticket to handle this later.

mlabisi · 2024-05-17T00:40:06Z

src/graphql.rs

@@ -3641,22 +3684,33 @@ impl ___Type for FilterEntityType {
                        not_column_exists = true;
                    }

-                    match utype.unmodified_type() {
-                        __Type::Scalar(s) => Some(__InputValue {
+                    match utype.nullable_type() {


Now that we support filtering by array columns, we want to respect the fact that the utype is an array, so we'll only unwrap Non-Nullables.

mlabisi · 2024-05-17T00:53:48Z

This one's ready for review whenever you have a chance @olirice @imor !

mlabisi · 2024-05-17T00:56:59Z

~~(Oops, just realized I forgot to update the docs—working on that now!)~~ Done!

mlabisi · 2024-05-21T17:10:36Z

Sorry to pester, but it'd be great to get any feedback on this PR! @olirice

olirice

at a glance the PR looks very promising!

unfortunately I'm about to be traveling until June 3 and likely won't have a chance to look through it until then. This is a large PR so I'd like to make sure we're thorough

A couple of quick comments. Please:

remove the version bump, we'll do that separately
avoid unwrap everywhere
user exhaustive pattern matching, even when it feels illogical (so we can rely on the compiler to identify all references to enum types)

great work. looking forward to reviewing it once back in office & sorry for the delay!

mlabisi · 2024-05-22T06:20:20Z

@olirice no worries, safe travels! I'll address your feedback in the meantime. Thank you!!

imor

Thank you @mlabisi for this PR, this looks great. I've left some minor actionable comments.

src/graphql.rs

imor · 2024-05-22T06:17:55Z

src/graphql.rs

@@ -3541,6 +3555,8 @@ impl ___Type for FilterTypeType {
                            default_value: None,
                            sql_type: None,
                        },
+                        // shouldn't happen since we've covered all cases in supported_ops
+                        _ => panic!("encountered unknown FilterOp")


Maybe we should restructure the FilterOp and related types so that we can avoid this catchall panic. As this PR is already long enough, I'm ok with doing that as a separate PR. Just open a ticket to handle this later.

test/expected/resolve_connection_filter.out

test/sql/resolve_connection_filter.sql

imor · 2024-05-27T07:09:25Z

@olirice the code looks ready to merge. Needs your approval to merge. @mlabisi thanks again for this contribution.

leefordjudes · 2024-05-31T06:56:57Z

Kindly merge this pr. this feature is need to my project.

Oli has been away, and in the interest of merging quickly, I'm dismissing this review.

mlabisi · 2024-05-31T18:41:51Z

Thanks for merging, @imor! I use Supabase Cloud, so I'm excited to see these changes go live! Are you able to share when Supabase Cloud will get these changes?

imor · 2024-06-03T07:01:38Z

@mlabisi please open a ticket with support to get your project updated to v1.5.5, which is currently being released: https://github.com/supabase/pg_graphql/releases/tag/v1.5.5

mlabisi · 2024-06-03T14:59:59Z

@mlabisi please open a ticket with support to get your project updated to v1.5.5, which is currently being released: https://github.com/supabase/pg_graphql/releases/tag/v1.5.5

@imor Will do, thanks so much!

mlabisi added 14 commits May 14, 2024 14:37

adding mock blog posts to seed data

a013f22

adding list type to filterable types

793254a

adding array filterops

4f08594

using modified type for match

fb61169

adding array filter types to transpiler

163707e

adding array filtering to builder

91b6b17

fixing array filter input fields

c425f8f

adding list filter types to schema

237a0a8

correcting entity name for list filter

809047a

fixup! adding mock blog posts to seed data

ee149f2

simplifying array filter builders

9851b3c

un-nesting list filter

4cc6752

unwrapping non-nullable types

201154d

enabling ordering by array columns

a080c4f

olirice reviewed May 15, 2024

View reviewed changes

mlabisi requested a review from olirice May 15, 2024 17:02

mlabisi changed the title ~~(WIP ⌛ ) Adding support for filtering and ordering on array column types~~ (WIP ⌛ ) Adding support for filtering on array column types May 15, 2024

disabling ordering by array columns

0a45e97

mlabisi added 4 commits May 15, 2024 20:20

updating failing tests

83c5ea9

adding test cases for filtering by array columns

384e394

removing archived status from seed data

a792bc1

removing extra comment from seed data

c2ee53d

mlabisi commented May 17, 2024

View reviewed changes

mlabisi added 2 commits May 16, 2024 17:32

removing unnecessary array handling from create_filters

dd9784a

removing unnecessary spacing change from create_filters

498d35c

mlabisi commented May 17, 2024

View reviewed changes

src/graphql.rs Outdated Show resolved Hide resolved

mlabisi commented May 17, 2024

View reviewed changes

mlabisi changed the title ~~(WIP ⌛ ) Adding support for filtering on array column types~~ Adding support for filtering on array column types May 17, 2024

mlabisi marked this pull request as ready for review May 17, 2024 00:43

mlabisi added 5 commits May 16, 2024 19:22

updating docs to include array column filters

26be47c

adding missing result to docs

761f226

bumping minor version

e658d83

bumping patch version

3e3fb84

Merge remote-tracking branch 'origin/issue-460' into issue-460

86b11fe

olirice previously requested changes May 21, 2024

View reviewed changes

imor reviewed May 22, 2024

View reviewed changes

mlabisi added 4 commits May 25, 2024 17:51

removing bump on patch version

2e2a6be

removing unwrap

c1a4ebe

Merge branch 'refs/heads/master' into issue-460

02c273f

correcting ov test

ca2890b

mlabisi requested review from olirice and imor May 26, 2024 01:16

correcting wording for cd docs

edcf8e0

imor approved these changes May 27, 2024

View reviewed changes

imor merged commit 53f0a9d into supabase:master May 31, 2024
4 checks passed

FelixMalfait mentioned this pull request Jun 12, 2024

Multi Select cannot filter twentyhq/twenty#5358

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for filtering on array column types #520

Adding support for filtering on array column types #520

mlabisi commented May 15, 2024 •

edited

Loading

olirice left a comment

mlabisi commented May 15, 2024

olirice commented May 15, 2024

mlabisi commented May 15, 2024

mlabisi May 17, 2024

mlabisi May 17, 2024

imor May 22, 2024

mlabisi May 17, 2024

mlabisi commented May 17, 2024

mlabisi commented May 17, 2024 •

edited

Loading

mlabisi commented May 21, 2024

olirice left a comment

mlabisi commented May 22, 2024

imor left a comment

imor May 22, 2024

imor commented May 27, 2024

leefordjudes commented May 31, 2024

mlabisi commented May 31, 2024

imor commented Jun 3, 2024

mlabisi commented Jun 3, 2024

Adding support for filtering on array column types #520

Adding support for filtering on array column types #520

Conversation

mlabisi commented May 15, 2024 • edited Loading

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

Additional context

olirice left a comment

Choose a reason for hiding this comment

mlabisi commented May 15, 2024

olirice commented May 15, 2024

mlabisi commented May 15, 2024

mlabisi May 17, 2024

Choose a reason for hiding this comment

mlabisi May 17, 2024

Choose a reason for hiding this comment

imor May 22, 2024

Choose a reason for hiding this comment

mlabisi May 17, 2024

Choose a reason for hiding this comment

mlabisi commented May 17, 2024

mlabisi commented May 17, 2024 • edited Loading

mlabisi commented May 21, 2024

olirice left a comment

Choose a reason for hiding this comment

mlabisi commented May 22, 2024

imor left a comment

Choose a reason for hiding this comment

imor May 22, 2024

Choose a reason for hiding this comment

imor commented May 27, 2024

leefordjudes commented May 31, 2024

mlabisi commented May 31, 2024

imor commented Jun 3, 2024

mlabisi commented Jun 3, 2024

mlabisi commented May 15, 2024 •

edited

Loading

mlabisi commented May 17, 2024 •

edited

Loading