Opened 9 years ago
Closed 6 years ago
#12567 closed Bug (fixed)
Incorrect SQL being generated in certain inheritance cases.
Reported by: | Alex Gaynor | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | master |
Severity: | Normal | Keywords: | |
Cc: | Vlastimil Zíma | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Given the models
from django.db import models class A(models.Model): pass class B(A): pass class C(A): pass
The query:
>>> A.objects.filter(b__isnull=False)
Generates bad SQL (specifically it omits the needed join). Worth noting that:
>>> A.objects.exclude(b=None)
works correctly.
Attachments (3)
Change History (18)
Changed 9 years ago by
Attachment: | testcase.diff added |
---|
comment:1 Changed 9 years ago by
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 Changed 9 years ago by
Summary: | Invalid SQL being generated in certain inheritance cases. → Incorrect SQL being generated in certain inheritance cases. |
---|
Changed the summary, it wasn't totally accurate.
comment:3 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:4 Changed 9 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Sorry, closed wrong ticket.
comment:5 Changed 9 years ago by
For reference, this is the SQL generated by the two operations:
A.objects.filter(b__isnull=False) SELECT "test_app_a"."id", "test_app_a"."a_field" FROM "test_app_a" WHERE "test_app_a"."id" IS NOT NULL LIMIT 21 A.objects.exclude(b=None) SELECT "test_app_a"."id", "test_app_a"."a_field" FROM "test_app_a" WHERE NOT ( "test_app_a"."id" IN ( SELECT U0."id" FROM "test_app_a" U0 LEFT OUTER JOIN "test_app_b" U1 ON (U0."id" = U1."a_ptr_id") WHERE U1."a_ptr_id" IS NULL ) ) LIMIT 21
comment:6 Changed 9 years ago by
Has patch: | set |
---|
So there is a bit in the query's add_filter() method that blocks join promotion if value != True. value == False seems like a valid expression, as the above example illustrates, so I simply removed that clause. I included Alex's test case in the diff.
Changed 9 years ago by
Attachment: | 12567.diff added |
---|
comment:7 Changed 9 years ago by
Alex pointed out that switching the promote_alias_chain bit to promote all isnull lookups would have a bad effect on queries, as isnull=False is way fast on an inner join. I poked around in trim_joins() and special-cased the break in the while loop, but ended up with some failing regressions tests generally when Q objects and pipes were used with querysets.
I ended up by choosing to skip the trim_joins() step altogether if it was an isnull=False lookup with joins. All tests are passing, including the one alex provided, and as you can see, the joins are being evaluated correctly:
In [10]: A.objects.exclude(b=None) Out[10]: [<A: A object>] In [11]: connection.queries.pop() Out[11]: {'sql': u'SELECT "test_app_a"."id", "test_app_a"."a_field" FROM "test_app_a" WHERE NOT ("test_app_a"."id" IN (SELECT U0."id" FROM "test_app_a" U0 LEFT OUTER JOIN "test_app_b" U1 ON (U0."id" = U1."a_ptr_id") WHERE U1."a_ptr_id" IS NULL)) LIMIT 21', 'time': '0.000'} In [12]: A.objects.filter(b__isnull=False) Out[12]: [<A: A object>] In [13]: connection.queries.pop() Out[13]: {'sql': u'SELECT "test_app_a"."id", "test_app_a"."a_field" FROM "test_app_a" INNER JOIN "test_app_b" ON ("test_app_a"."id" = "test_app_b"."a_ptr_id") WHERE "test_app_b"."a_ptr_id" IS NOT NULL LIMIT 21', 'time': '0.000'}
Changed 9 years ago by
Attachment: | 12567.2.diff added |
---|
updated patch to respect query optimization on isnull=False & inner joins
comment:8 Changed 9 years ago by
milestone: | 1.2 → 1.3 |
---|
There's a lot of similarity here with #12153; I don't think it's the same problem, but it's in a very similar area.
Either way, Not critical for 1.2
comment:9 Changed 8 years ago by
Cc: | Vlastimil Zíma added |
---|
comment:10 Changed 8 years ago by
milestone: | 1.3 |
---|---|
Severity: | → Normal |
Type: | → Bug |
comment:11 Changed 8 years ago by
Patch needs improvement: | set |
---|
The tests would need to be rewritten using unittests since this is now Django's preferred way.
comment:14 Changed 6 years ago by
Status: | reopened → new |
---|
comment:15 Changed 6 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
a test case