Opened 11 years ago

Last modified 5 years ago

#20535 new Bug

Unnecessary join created for intermediate table between two M2M tables

Reported by: German M. Bravo Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Zach Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In this example:

from django.db import models

class Person(models.Model):
    name = models.CharField(max_length=128)
    friends = models.ManyToManyField('self', symmetrical=False, through='Relationship')

    def __unicode__(self):
        return self.name

class Relationship(models.Model):
    from_person = models.ForeignKey(Person, related_name='idols')
    to_person = models.ForeignKey(Person, related_name='followers')

Example data for a test:

ringo, _ = Person.objects.get_or_create(name="Ringo Starr")
paul, _ = Person.objects.get_or_create(name="Paul McCartney")
george, _ = Person.objects.get_or_create(name="George Harrison")
john, _ = Person.objects.get_or_create(name="John Lennon")
yoko, _ = Person.objects.get_or_create(name="Yoko Ono")

Relationship.objects.get_or_create(from_person=ringo, to_person=paul)
Relationship.objects.get_or_create(from_person=george, to_person=john)
Relationship.objects.get_or_create(from_person=paul, to_person=ringo)
Relationship.objects.get_or_create(from_person=paul, to_person=george)
Relationship.objects.get_or_create(from_person=paul, to_person=john)
Relationship.objects.get_or_create(from_person=john, to_person=paul)
Relationship.objects.get_or_create(from_person=john, to_person=yoko)
Relationship.objects.get_or_create(from_person=yoko, to_person=john)

Then doing (get idols of the idols of paul):

    print Relationship.objects.filter(from_person__followers__from_person=paul).query

This produces a join between Relationship and Person, then to Relationship, then to Person. The final join (to Person) gets trimmed by trim_joins() after the setup_joins() in Query.add_filter(). However, the unneeded join with Person (between the two joined Relationship) is not removed. This produces something like:

SELECT ... FROM relationship INNER JOIN person ON (relationship.from_person_id = person.id) INNER JOIN relationship T3 ON (person.id = T3.to_person_id) WHERE T3.from_person_id = 2;

When in fact all that would be needed is:

SELECT ... FROM relationship INNER JOIN relationship T3 ON (relationship.from_person_id = T3.to_person_id) WHERE T3.from_person_id = 2

Attachments (4)

#20535-tests.diff (1.4 KB ) - added by German M. Bravo 11 years ago.
#20535-tests_master.diff (572 bytes ) - added by German M. Bravo 11 years ago.
Tests for master
#20353-Unnecessary_join created for intermediate_table_between_two_M2M_tables.diff (3.5 KB ) - added by German M. Bravo 11 years ago.
Patch with tests
#20535-Unnecessary_join created for intermediate_table_between_two_M2M_tables.diff (3.4 KB ) - added by German M. Bravo 11 years ago.

Download all attachments as: .zip

Change History (8)

by German M. Bravo, 11 years ago

Attachment: #20535-tests.diff added

by German M. Bravo, 11 years ago

Attachment: #20535-tests_master.diff added

Tests for master

comment:1 by German M. Bravo, 11 years ago

Has patch: set

comment:2 by German M. Bravo, 11 years ago

Version: 1.5master

comment:3 by Anssi Kääriäinen, 11 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Yes, skipping non-necessary intermediate joins would be good. Unfortunately the added patch seems to introduce a lot of errors in the test suite.

Maybe it would be better to do the trimming at compiler.get_from_clause() time? A join can be skipped if there are as many tables joining to it as its alias_refcount is, and every joined table can be joined directly to the "parent" table instead.

Trimming joins before joins promotion is done is a bad idea. I am not sure if this applies to internal join trimming though.

comment:4 by Zach, 6 years ago

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