Opened 5 years ago

Last modified 14 months ago

#16603 new Bug

Unnecessary join when using a reverse foreign-key filter and reverse foreign-key aggregate call

Reported by: Ben Davis Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: anssi.kaariainen@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

(This started as #16554, but it was only half-valid, so I'm re-submitting the valid part here)

Django allows you to perform queries across reverse foreign key relationships. If, however, you need to access that same relationship in a filter and an aggregate call, the ORM creates unnecessary joins.

Example:

app/models.py

class Author(models.Model):
    first_name = models.CharField(max_length=30)
    last_name = models.CharField(max_length=30)
    email = models.EmailField()

class Book(models.Model):
    author = models.ForeignKey(Author, related_name='books')
    title = models.CharField(max_length=100)
    genre = models.CharField(max_length=20, choices=(
        ('SCIFI', 'SciFi'),
        ('FANTASY', 'Fantasy'),
        ('NONFICTION', 'NonFiction')
    ))
    published = models.DateField()
    pages = models.IntegerField()

The django query:

qs = Author.objects.annotate(pages_written=Sum('books__pages'))
qs = qs.filter(books__genre='SCIFI')
print qs.query

Generated SQL:

SELECT "app_author"."id", "app_author"."first_name", "app_author"."last_name", "app_author"."email", SUM("app_book"."pages") AS "pages_written" 
FROM "app_author" 
LEFT OUTER JOIN "app_book" ON ("app_author"."id" = "app_book"."author_id") 
INNER JOIN "app_book" T3 ON ("app_author"."id" = T3."author_id") 
WHERE T3."genre" = 'SCIFI'
GROUP BY "app_author"."id", "app_author"."first_name", "app_author"."last_name", "app_author"."email"

Attachments (2)

16603_tests.patch (993 bytes) - added by Anssi Kääriäinen 5 years ago.
16603-test.diff (1022 bytes) - added by Tim Graham 14 months ago.

Download all attachments as: .zip

Change History (6)

Changed 5 years ago by Anssi Kääriäinen

Attachment: 16603_tests.patch added

comment:1 Changed 5 years ago by Anssi Kääriäinen

Component: Database layer (models, ORM)ORM aggregation
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

Confirmed. This is somewhat related to #10060, but not a duplicate of that ticket. I am a bit surprised if there is not a duplicate of this ticket, but I could not find any. Maybe I am just too tired...

The problem reported is not easy to fix. The main issue is that reverse (and m2m) filtering should be done using subqueries and WHERE EXISTS, WHERE pk IN or joining the subquery. As implemented now the joins generated will return duplicate rows, and that causes the problem. That is, the above query should be written as

SELECT ... 
FROM "app_author" 
LEFT OUTER JOIN "app_book" ON ("app_author"."id" = "app_book"."author_id")  
WHERE EXISTS (SELECT 1 FROM app_book T3 WHERE T3.author_id = app_author.id and t3.genre = 'SCIFI')
GROUP BY ...

or

SELECT ... 
FROM "app_author" 
LEFT OUTER JOIN "app_book" ON ("app_author"."id" = "app_book"."author_id") 
WHERE app_author.id IN (SELECT author_id FROM app_book T3 WHERE t3.genre = 'SCIFI')
GROUP BY ...

or

SELECT ... 
FROM "app_author" 
LEFT OUTER JOIN "app_book" ON ("app_author"."id" = "app_book"."author_id") 
JOIN (SELECT author_id FROM app_book T3 WHERE t3.genre = 'SCIFI') T5 ON ("app_author"."id" = T5."author_id") 
GROUP BY ...

I haven't tested the queries above, and as said I am a little tired... But the idea should at least be correct :)

comment:2 Changed 5 years ago by Anssi Kääriäinen

Cc: anssi.kaariainen@… added

comment:3 Changed 4 years ago by Anssi Kääriäinen

Component: ORM aggregationDatabase layer (models, ORM)

comment:4 Changed 14 months ago by Tim Graham

Updated test to apply cleanly and verified it's still an issue as of 98bcdfa8bd902addd4b8cf37d039b3597d58a45c.

Changed 14 months ago by Tim Graham

Attachment: 16603-test.diff added
Note: See TracTickets for help on using tickets.
Back to Top