Opened 8 years ago

Closed 11 months ago

Last modified 11 months ago

#10045 closed Cleanup/optimization (fixed)

Improve documentation of .annotate() / .filter() ordering quirks

Reported by: alex@… Owned by: Tim Graham <timograham@…>
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Russell Keith-Magee)

The documentation surrounding the ordering of .annotate() and .filter() clauses needs improvement. The current documentation is confusing (and arguably incorrect), leading to invalid bug reports like the following:

---

I get something that looks like a missing join condition when doing

ModelA.objects.all().annotate(num_b=Count('modelb')).filter(modelb__somebool=True)[0].num_b

I get num_b = 2^i for i objects of ModelB instead of num_b=i as I would expect to.
Please see attached models.py file for the Model definition and test case.

django revision: 9756

---

Attachments (2)

models.py (709 bytes) - added by alex@… 8 years ago.
test_10045.tar.gz (6.4 KB) - added by Chris Beaven 5 years ago.
self-contained simple project - run manage.py test

Download all attachments as: .zip

Change History (13)

Changed 8 years ago by alex@…

Attachment: models.py added

comment:1 Changed 8 years ago by Russell Keith-Magee

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Resolution: invalid
Status: newclosed

I think you may have missed this section of the docs:

There is a difference between:

ModelA.objects.all().annotate(num_b=Count('modelb')).filter(modelb__somebool=True)[0].num_b

which evaluates as the SQL:

SELECT "myapp_modela"."id", "myapp_modela"."name", COUNT("myapp_modelb"."id") AS "num_b" FROM "myapp_modela" LEFT OUTER JOIN "myapp_modelb" ON ("myapp_modela"."id" = "myapp_modelb"."a_id") INNER JOIN "myapp_modelb" T3 ON ("myapp_modela"."id" = T3."a_id") WHERE T3."somebool" = True  GROUP BY "myapp_modela"."id", "myapp_modela"."name"

and the query:

ModelA.objects.all().filter(modelb__somebool=True).annotate(num_b=Count('modelb'))[0].num_b

which evaluates as the SQL:

SELECT "myapp_modela"."id", "myapp_modela"."name", COUNT("myapp_modelb"."id") AS "num_b" FROM "myapp_modela" LEFT OUTER JOIN "myapp_modelb" ON ("myapp_modela"."id" = "myapp_modelb"."a_id") WHERE "myapp_modelb"."somebool" = True  GROUP BY "myapp_modela"."id", "myapp_modela"."name"

Notice the order of the annotate() and filter() clauses. By putting the filter after the annotate, the annotation uses a separate join table. When the annotate follows the filter, the annotation uses the filter's join table.

comment:2 Changed 8 years ago by alex@…

I did read the docs, but apparently didn't understand them: They say: "annotation in the first query will provide the total number of all books published by the publisher;" that would map to the total number of B objects in my example, wouldn't it? And that is not 4. Also: "In the first query, the annotation precedes the filter, so the filter has no effect on the annotation".
Remove the filter, and the expected result is returned:

>>> ModelA.objects.all().annotate(num_b=Count('modelb'))[0].num_b
2

Note that somebool is True on all ModelB objects, so the filter should really have no effect, right?

comment:3 Changed 8 years ago by Russell Keith-Magee

Component: ORM aggregationDocumentation
Description: modified (diff)
Resolution: invalid
Status: closedreopened
Summary: bug in aggregations: .annotate() followed by .filter() on related field breaksImprove documentation of .annotate() / .filter() ordering quirks
Triage Stage: UnreviewedAccepted

In this case, I am confirming that the query is returning the correct results. The order of filter() and annotate() is significant, and the result you have received is what should be expected.

However, I will concede that the documentation of this quirk could certainly be improved. I'll reopen this ticket, but I'll repurpose it to clarify the documentation of this feature since there isn't anything at a code level that requires fixing. If you (or anyone else) wants to take a swing at describing this quirk better, feel free to do so.

comment:4 Changed 5 years ago by Chris Beaven

Severity: Normal
Type: Cleanup/optimization

comment:5 Changed 5 years ago by anonymous

Easy pickings: unset
UI/UX: unset

This seems more than documenting a "quirk". The very example given in the docs is broken.

Changed 5 years ago by Chris Beaven

Attachment: test_10045.tar.gz added

self-contained simple project - run manage.py test

comment:6 Changed 5 years ago by Chris Beaven

(anon was me)

In the project test, I create two Publishers, and assign three Books to the first publisher, two above a rating of 3.0. Running Publisher.objects.annotate(num_books=Count('book')).filter(book__rating__gt=3.0)[0].num_books returns 6.

comment:7 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:8 Changed 11 months ago by Tim Graham

Has patch: set

PR -- I could use some advice from ORM experts about how to explain the actual behavior. I'm not sure if it might be a bug that we can fix later or if we should simply say "don't do that."

comment:9 Changed 11 months ago by Tim Graham <timograham@…>

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In 91a431f4:

Fixed #10045 -- Corrected docs about .annotate()/.filter() ordering.

Thanks Josh, Anssi, and Carl for reviews and advice.

comment:10 Changed 11 months ago by Tim Graham <timograham@…>

In 16865782:

[1.9.x] Fixed #10045 -- Corrected docs about .annotate()/.filter() ordering.

Thanks Josh, Anssi, and Carl for reviews and advice.

Backport of 91a431f48c1fc5ecc9a837e8071a0062d31b490f from master

comment:11 Changed 11 months ago by Tim Graham <timograham@…>

In 10d18dec:

[1.8.x] Fixed #10045 -- Corrected docs about .annotate()/.filter() ordering.

Thanks Josh, Anssi, and Carl for reviews and advice.

Backport of 91a431f48c1fc5ecc9a837e8071a0062d31b490f from master

Note: See TracTickets for help on using tickets.
Back to Top