Opened 6 years ago

Closed 2 years ago

#12567 closed Bug (fixed)

Incorrect SQL being generated in certain inheritance cases.

Reported by: Alex Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: vzima 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)

testcase.diff (960 bytes) - added by Alex 6 years ago.
a test case
12567.diff (1.6 KB) - added by coleifer 6 years ago.
12567.2.diff (1.8 KB) - added by coleifer 6 years ago.
updated patch to respect query optimization on isnull=False & inner joins

Download all attachments as: .zip

Change History (18)

Changed 6 years ago by Alex

a test case

comment:1 Changed 6 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 6 years ago by Alex

  • Summary changed from Invalid SQL being generated in certain inheritance cases. to Incorrect SQL being generated in certain inheritance cases.

Changed the summary, it wasn't totally accurate.

comment:3 Changed 6 years ago by zgoda

  • Resolution set to fixed
  • Status changed from new to closed

(In [12205]) Fixed #12567: updated Polish translation of GIS terms. Thanks pigletto for patch.

comment:4 Changed 6 years ago by zgoda

  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry, closed wrong ticket.

comment:5 Changed 6 years ago by coleifer

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 6 years ago by coleifer

  • 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 6 years ago by coleifer

comment:7 Changed 6 years ago by coleifer

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 6 years ago by coleifer

updated patch to respect query optimization on isnull=False & inner joins

comment:8 Changed 5 years ago by russellm

  • milestone changed from 1.2 to 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 5 years ago by vzima

  • Cc vzima added

comment:10 Changed 4 years ago by mattmcc

  • milestone 1.3 deleted
  • Severity set to Normal
  • Type set to Bug

comment:11 Changed 4 years ago by julien

  • Patch needs improvement set

The tests would need to be rewritten using unittests since this is now Django's preferred way.

comment:12 Changed 4 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:13 Changed 4 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:14 Changed 2 years ago by aaugustin

  • Status changed from reopened to new

comment:15 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 630b9df42f771e90d9beb1766d4e7aa2107bd82d:

Fixed #12567 -- Incorrect SQL in model inheritance case

An isnull lookup produced incorrect SQL. This was already fixed
earlier, so only tests added.

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