-
Notifications
You must be signed in to change notification settings - Fork 83
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 optimizing querysets with annotations #35
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #35 +/- ##
==========================================
+ Coverage 93.54% 93.82% +0.27%
==========================================
Files 6 6
Lines 310 324 +14
==========================================
+ Hits 290 304 +14
Misses 20 20
Continue to review full report at Codecov.
|
Bumps [django](https://github.com/django/django) from 2.2.2 to 2.2.4. - [Release notes](https://github.com/django/django/releases) - [Commits](django/django@2.2.2...2.2.4) Signed-off-by: dependabot[bot] <[email protected]>
Bump django from 2.2.2 to 2.2.4
Hi! We are sorry, we didn't have time yet to fully test the changes. But we would like to let you know we didn't forget about your pull request and we really appreciate your suggestions! Great work! |
Great contribution. Just tested it and it works. One thing I've noticed is that if you use two fields in a GraphQL query that are using annotations with aggregations at the same time you will get wrong data. (Combining multiple aggregations) It shouldn't be possible for the client to get wrong data by combining different fields. Maybe this could be avoided by throwing an exceptions if two fields with annotations and aggregations are used at the same time. |
I've just tested this on a project (with a forked version against latest master), and it's fantastic. Trying to support optimized annotations "by hand" without these additions is incredibly tough. Thank you hilmarh for opening this pull request! Is there any chance it could be considered now and merged? This is an old PR but is highly useful still. The "combining multiple aggregations" issue mentioned above seems unavoidable per Django's notes, so throwing an error seems like a reasonable option, if desired. |
Hi @sjdemartini, can you provide an example of using this vs not having this feature? |
Thanks for following up, tfoxy! I don't have the code handy anymore when I'd attempted to build this myself outside of the gql_optimizer (without using this branch's code), but I never ended up getting it working as well as this. (e.g. if the model with the query-annotation wasn't the top-level field/list requested in the response, but instead was a child of the main field requested, getting the optimization to work properly and avoid N+1 queries proved really tricky.) For what it's worth, I ended up personally moving away from this approach, and instead now make two separate queries, where the second query fetches the data that would've been included via annotation. |
@hilmar8 you just made my week. |
@tfoxy this is incredibly useful! |
I've been using my own fork of this library for some time alongside my own fork of graphene-django and I'm ready to merge some of the changes back that I've been using in production successfully.
For this pull request I suggest annotations.
The biggest caveat, and why annotations should be listed as an advanced feature, is that they stop working if the optimization fails for other reasons. In my projects I always have tests that use
django_assert_num_queries
and always check that the optimization hasn't failed, like:assert "updated_at" not in connection.queries[0]["sql"]
.Even though annotations could be easy to break, they make this library much more powerful. An example using GeoDjango features, only calculating
distance
if it's requested. Saving an extra PostGIS calculation when not needed.Other useful examples can be seen in the tests and README. Most useful ones being
Sum
andCount
.