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

Support limit queries with Dataloader.Ecto #93

Merged
merged 13 commits into from
Jul 3, 2020

Conversation

mbuhot
Copy link
Contributor

@mbuhot mbuhot commented Jun 19, 2020

This PR is a proof of concept for implementing #51

With Ecto now supporting parent_as, lateral joins are now possible without fragments.

The implementation loads the inputs into a CTE to drive a lateral join query.
Unfortunately it is using the postgres-specific unnest function, which also requires explicitly casting the inputs to an array type. Any suggestions on making it more portable/general?

Edit: Found a way to make it work without fragments or casting.

Update: I've added support for preloading associations using lateral joins too, this makes it very convenient to use from absinthe resolvers using the dataloader/1 resolution helper.

Queries that do not rely on limit/offset continue to use
the existing approach, where a simple where clause is added
to the query.

Complex queries are converted into a lateral subquery.
The outer query contains the inputs.
Ideally the query would use `unnest` or a similar function
to efficiently load an array of values into a query, but
Ecto doesn't offer such functionality in a portable way.

The workaround here is to run a distinct query that selects
the column from the queryable where the value is any of the
inputs.
@benwilson512
Copy link
Contributor

This is amazing, fantastic work. Gonna mess with it some. Can you include a test case or two?

@mbuhot
Copy link
Contributor Author

mbuhot commented Jun 23, 2020

Thanks @benwilson512!

I've been prototyping with this app based on the Chinook dataset: https://github.com/mbuhot/chinook/blob/master/test/chinook_web/graphql/playlist_test.exs

It works wonderfully to declare nested relay connections with a 1 line resolution helper :)

  node object(:playlist) do
    field :name, non_null(:string)

    connection field :tracks, node_type: :track do
      arg :by, :track_sort_order, default_value: :track_id
      arg :filter, :track_filter, default_value: %{}
      resolve Relay.connection_dataloader(Chinook.Track.Loader) # resolves through a many-to-many relationship
    end
  end

I'll add some tests that exercise some of the variations on HasMany, HasThrough, ManyToMany.

@benwilson512
Copy link
Contributor

Incredible! I'll pull this down today and mess with it but this is looking basically perfect so far.

This can happen when there is a through association like
A -has many-> B -belongs to-> C

When loading the A -> C association using a join, duplicate entries for C
were being returned.

The fix is to throw a distinct() on the inner query.
There may be a more elegant approach to detecting when
the distinct is necessary, but at least the results are correct now.
When A HasMany B and B BelongsTo C, then A can have the same C multiple times.
A ManyToMany association is similar.
This unfortunately required a significant duplication of the
build_preload_query code.

When the users query contains joins, the first join from the users
query to an association must target the first source in the query,
not the last one.
@mbuhot
Copy link
Contributor Author

mbuhot commented Jun 28, 2020

@benwilson512 I've added some more code and tests to cover some edge cases I ran into.

Let me know if the code can be better organised. I'm starting to wonder if the preload_lateral machinery would be better in Ecto itself rather than in Dataloader.Ecto 🤔

@benwilson512
Copy link
Contributor

I'm starting to wonder if the preload_lateral machinery would be better in Ecto itself rather than in Dataloader.Ecto

Agreed. Perhaps see if there is interest for a PR on ecto to add that. If not we can go ahead and just add it here.

@benwilson512
Copy link
Contributor

@mbuhot one thing I'm noticing is that Dataloader still specifies its ecto dependency as >= 0.0.0. Can you make that more precise to whatever version adds the preload support you require?

@mbuhot
Copy link
Contributor Author

mbuhot commented Jul 2, 2020

Will do 👍 3.4.3 provides parent_as which is the key to it.

@benwilson512 benwilson512 merged commit 5bdf294 into absinthe-graphql:master Jul 3, 2020
@benwilson512
Copy link
Contributor

Thank you!

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.

2 participants