Skip to content

Commit

Permalink
Fixed #32143 -- Used EXISTS to exclude multi-valued relationships.
Browse files Browse the repository at this point in the history
As mentioned in the pre-existing split_exclude() docstring EXISTS is
easier to optimize for query planers and circumvents the IN (NULL)
handling issue.
  • Loading branch information
charettes authored and felixxm committed Oct 28, 2020
1 parent bbf141b commit 8593e16
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 25 deletions.
9 changes: 4 additions & 5 deletions django/db/models/expressions.py
Expand Up @@ -1083,19 +1083,18 @@ class Subquery(Expression):
contains_aggregate = False

def __init__(self, queryset, output_field=None, **extra):
self.query = queryset.query
# Allow the usage of both QuerySet and sql.Query objects.
self.query = getattr(queryset, 'query', queryset)
self.extra = extra
# Prevent the QuerySet from being evaluated.
self.queryset = queryset._chain(_result_cache=[], prefetch_done=True)
super().__init__(output_field)

def __getstate__(self):
state = super().__getstate__()
args, kwargs = state['_constructor_args']
if args:
args = (self.queryset, *args[1:])
args = (self.query, *args[1:])
else:
kwargs['queryset'] = self.queryset
kwargs['queryset'] = self.query
state['_constructor_args'] = args, kwargs
return state

Expand Down
32 changes: 14 additions & 18 deletions django/db/models/sql/query.py
Expand Up @@ -23,7 +23,9 @@
from django.db import DEFAULT_DB_ALIAS, NotSupportedError, connections
from django.db.models.aggregates import Count
from django.db.models.constants import LOOKUP_SEP
from django.db.models.expressions import BaseExpression, Col, F, OuterRef, Ref
from django.db.models.expressions import (
BaseExpression, Col, Exists, F, OuterRef, Ref, ResolvedOuterRef,
)
from django.db.models.fields import Field
from django.db.models.fields.related_lookups import MultiColSource
from django.db.models.lookups import Lookup
Expand Down Expand Up @@ -1765,12 +1767,12 @@ def split_exclude(self, filter_expr, can_reuse, names_with_path):
filters in the original query.
We will turn this into equivalent of:
WHERE NOT (pk IN (SELECT parent_id FROM thetable
WHERE name = 'foo' AND parent_id IS NOT NULL))
It might be worth it to consider using WHERE NOT EXISTS as that has
saner null handling, and is easier for the backend's optimizer to
handle.
WHERE NOT EXISTS(
SELECT 1
FROM child
WHERE name = 'foo' AND child.parent_id = parent.id
LIMIT 1
)
"""
filter_lhs, filter_rhs = filter_expr
if isinstance(filter_rhs, OuterRef):
Expand All @@ -1786,17 +1788,9 @@ def split_exclude(self, filter_expr, can_reuse, names_with_path):
# the subquery.
trimmed_prefix, contains_louter = query.trim_start(names_with_path)

# Add extra check to make sure the selected field will not be null
# since we are adding an IN <subquery> clause. This prevents the
# database from tripping over IN (...,NULL,...) selects and returning
# nothing
col = query.select[0]
select_field = col.target
alias = col.alias
if self.is_nullable(select_field):
lookup_class = select_field.get_lookup('isnull')
lookup = lookup_class(select_field.get_col(alias), False)
query.where.add(lookup, AND)
if alias in can_reuse:
pk = select_field.model._meta.pk
# Need to add a restriction so that outer query's filters are in effect for
Expand All @@ -1810,9 +1804,11 @@ def split_exclude(self, filter_expr, can_reuse, names_with_path):
query.where.add(lookup, AND)
query.external_aliases[alias] = True

condition, needed_inner = self.build_filter(
('%s__in' % trimmed_prefix, query),
current_negated=True, branch_negated=True, can_reuse=can_reuse)
lookup_class = select_field.get_lookup('exact')
lookup = lookup_class(col, ResolvedOuterRef(trimmed_prefix))
query.where.add(lookup, AND)
condition, needed_inner = self.build_filter(Exists(query))

if contains_louter:
or_null_condition, _ = self.build_filter(
('%s__isnull' % trimmed_prefix, True),
Expand Down
12 changes: 10 additions & 2 deletions tests/queries/tests.py
Expand Up @@ -2807,11 +2807,11 @@ def setUpTestData(cls):
f1 = Food.objects.create(name='apples')
Food.objects.create(name='oranges')
Eaten.objects.create(food=f1, meal='dinner')
j1 = Job.objects.create(name='Manager')
cls.j1 = Job.objects.create(name='Manager')
cls.r1 = Responsibility.objects.create(description='Playing golf')
j2 = Job.objects.create(name='Programmer')
r2 = Responsibility.objects.create(description='Programming')
JobResponsibilities.objects.create(job=j1, responsibility=cls.r1)
JobResponsibilities.objects.create(job=cls.j1, responsibility=cls.r1)
JobResponsibilities.objects.create(job=j2, responsibility=r2)

def test_to_field(self):
Expand Down Expand Up @@ -2884,6 +2884,14 @@ def test_exclude_nullable_fields(self):
[number],
)

def test_exclude_multivalued_exists(self):
with CaptureQueriesContext(connection) as captured_queries:
self.assertSequenceEqual(
Job.objects.exclude(responsibilities__description='Programming'),
[self.j1],
)
self.assertIn('exists', captured_queries[0]['sql'].lower())


class ExcludeTest17600(TestCase):
"""
Expand Down

0 comments on commit 8593e16

Please sign in to comment.