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

[WIP] Add Query Optimizer that selects fields from query AST #56

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

NathHorrigan
Copy link
Contributor

This is the initial code for optimizing GraphQL queries to reduce the number of database calls. The gist of this PR is that it analyses from the queries' AST what fields of what models it needs and then appends that to the Queryset object, further along the line when a .specific() call is applied these field selects are called on the Queryset using .only(..), .select_related(..) & .prefetch_related(..).

More accurate data will follow...

@NathHorrigan
Copy link
Contributor Author

What do you think @zerolab??

@@ -178,3 +179,7 @@
"ROUTING": "grapple.urls.channel_routing",
}
}

# Query Optimisation helpers
SHELL_PLUS_PRINT_SQL = True
Copy link
Member

Choose a reason for hiding this comment

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

these should be enabled only locally, maybe?

@@ -10,3 +10,4 @@ factory-boy==2.12.0
wagtail-factories==2.0.0
django-cors-headers==3.0.2
wagtail-headless-preview==0.1.4
django_extensions==2.2.9
Copy link
Member

Choose a reason for hiding this comment

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

I am not convinced we need the extra dependency. Should only be a dev dependency


if id is not None:
qs = qs.filter(pk=id)
qs = qs.get(id=id)
Copy link
Member

Choose a reason for hiding this comment

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

This can throw a DoesNotExist should the provided id not be there anymore

Copy link
Member

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Had a quick look and left a few comments.
Will try to review more in depth this weekend.
I am curious, do you have any before/after profiling stats?

incidentally, https://github.com/tolomea/django-auto-prefetch and https://github.com/jmcarp/nplusone popped up this week -- there some good ideas there

@fabienheureux
Copy link
Contributor

Why not consider using https://github.com/tfoxy/graphene-django-optimizer ?

@NathHorrigan
Copy link
Contributor Author

NathHorrigan commented May 5, 2020

Hi @fabienheureux , I did try GDQ but it doesn't work with stream field and that's a big issue for us. Instead of hack GDQ into Grapple, I decided to create our own way of doing it that can be fine tuned and expanded in the future.

@ruisaraiva19 ruisaraiva19 changed the title WIP: Add Query Optimizer that selects fields from query AST [WIP] Add Query Optimizer that selects fields from query AST Oct 5, 2020
@zerolab zerolab marked this pull request as draft October 7, 2020 10:55
@zerolab
Copy link
Member

zerolab commented Oct 29, 2020

@NathHorrigan any chance you can bring this to the finish line?

@ruisaraiva19
Copy link
Collaborator

ruisaraiva19 commented Nov 24, 2020

@NathHorrigan any chance you can bring this to the finish line?

@NathHorrigan any news on this PR? I'm going to fix the merge conflicts 🙂 .

Base automatically changed from master to main January 16, 2021 01:22
@fabienheureux
Copy link
Contributor

I started a PR on the graphene_django project to bring async resolvers support out of the box graphql-python/graphene-django#1256
It is still very much a work in progress, but I might be interested in restarting the work on optimizing queries with the addition of dataloaders to avoid n+1 queries once it has been merged 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants