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

Fix N+1 problem for one-to-many and many-to-many relationships #254

Merged
merged 4 commits into from
Jan 22, 2020

Conversation

jnak
Copy link
Collaborator

@jnak jnak commented Nov 19, 2019

This re-uses all the batching logic implemented in #253.

This optimization batches what used to be multiple SQL statements into a single SQL statement. While this this an improvement over the previous behavior, there is still some room for more optimizations:

  • We currently fetch all the children records of the relationship (vs just the ones being requested). Unfortunately, it's not possible to batch pagination with most SQL backends (see Join-Monster overview of the problem). This can gets quite complex so I think we should punt on that for now.
  • We currently fetch all the fields of the associated table (vs just the ones being queried). This should be relatively straightforward to implement (though there are some interesting edge cases) so I'm planning to address this soon.

For now, I decided to put this into a separate BatchSQLAlchemyConnectionField because the ...ConnectionField classes need to be refactored a bit in order to allow people to easily opt-out of the batching behavior if need be (via the ORMField mechanism). I'll be addressing this in a follow up PR soon. For now, you'll have to enable the optimization via the SQLAlchemyObjectType.Meta.connection_field_factory (see test_batching.py).

Cheers

@coveralls
Copy link

coveralls commented Nov 19, 2019

Coverage Status

Coverage increased (+0.06%) to 97.527% when pulling c0a447f on jnak:batch-sql-2 into 98e6fe7 on graphql-python:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 97.002% when pulling efafeb0 on jnak:batch-sql-2 into 98e6fe7 on graphql-python:master.

@Nabellaleen Nabellaleen self-requested a review November 24, 2019 11:47
@Nabellaleen
Copy link
Collaborator

The approach with using BatchSQLAlchemyConnectionField while the problems are not resolved, to allow a opt-in by the implementer is a good idea !

@jnak
Copy link
Collaborator Author

jnak commented Dec 27, 2019

@Nabellaleen Thanks for reviewing. Apologies for the not replying earlier - I got sidetracked by a concurrency bug in Promise and Dataloader that was breaking batching.

Once this PR is merged, I'm going to work on setting up benchmarks to compare with and without batching.

@jnak
Copy link
Collaborator Author

jnak commented Jan 6, 2020

I have 2 followup PRs that are currently blocked on this one:

I'll be sending those once this one is approved because AFAIK Github does not allow working with "stacked" PRs.

Cheers

@jnak jnak merged commit d90de4a into graphql-python:master Jan 22, 2020
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