Skip to content

Commit

Permalink
[3.0.x] Fixed #31150 -- Included subqueries that reference related fi…
Browse files Browse the repository at this point in the history
…elds in GROUP BY clauses.

Thanks Johannes Hoppe for the report.

Regression in fb3f034.

Co-authored-by: Simon Charette <charette.s@gmail.com>

Backport of 7b8fa16 from master
  • Loading branch information
felixxm committed Mar 3, 2020
1 parent 4977f20 commit c5cfaad
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 1 deletion.
15 changes: 14 additions & 1 deletion django/db/models/expressions.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from django.core.exceptions import EmptyResultSet, FieldError
from django.db import connection
from django.db.models import fields
from django.db.models.constants import LOOKUP_SEP
from django.db.models.query_utils import Q
from django.db.utils import NotSupportedError
from django.utils.deconstruct import deconstructible
Expand Down Expand Up @@ -558,6 +559,14 @@ def as_sql(self, *args, **kwargs):
'only be used in a subquery.'
)

def resolve_expression(self, *args, **kwargs):
col = super().resolve_expression(*args, **kwargs)
# FIXME: Rename possibly_multivalued to multivalued and fix detection
# for non-multivalued JOINs (e.g. foreign key fields). This should take
# into account only many-to-many and one-to-many relationships.
col.possibly_multivalued = LOOKUP_SEP in self.name
return col

def relabeled_clone(self, relabels):
return self

Expand Down Expand Up @@ -744,6 +753,7 @@ def as_sql(self, compiler, connection):
class Col(Expression):

contains_column_references = True
possibly_multivalued = False

def __init__(self, alias, target, output_field=None):
if output_field is None:
Expand Down Expand Up @@ -1068,7 +1078,10 @@ def as_sql(self, compiler, connection, template=None, **extra_context):
def get_group_by_cols(self, alias=None):
if alias:
return [Ref(alias, self)]
return self.query.get_external_cols()
external_cols = self.query.get_external_cols()
if any(col.possibly_multivalued for col in external_cols):
return [self]
return external_cols


class Exists(Subquery):
Expand Down
3 changes: 3 additions & 0 deletions docs/releases/3.0.4.txt
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,6 @@ Bugfixes
* Fixed a regression in Django 3.0.3 that caused misplacing parameters of SQL
queries when subtracting ``DateField`` or ``DateTimeField`` expressions on
MySQL (:ticket:`31312`).

* Fixed a regression in Django 3.0 that didn't include subqueries spanning
multivalued relations in the ``GROUP BY`` clause (:ticket:`31150`).
27 changes: 27 additions & 0 deletions tests/aggregation/tests.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import datetime
import re
from decimal import Decimal
from unittest import skipIf

from django.core.exceptions import FieldError
from django.db import connection
Expand Down Expand Up @@ -1189,6 +1190,26 @@ def test_aggregation_subquery_annotation_values(self):
},
])

@skipUnlessDBFeature('supports_subqueries_in_group_by')
@skipIf(
connection.vendor == 'mysql',
'GROUP BY optimization does not work properly when ONLY_FULL_GROUP_BY '
'mode is enabled on MySQL, see #31331.',
)
def test_aggregation_subquery_annotation_multivalued(self):
"""
Subquery annotations must be included in the GROUP BY if they use
potentially multivalued relations (contain the LOOKUP_SEP).
"""
subquery_qs = Author.objects.filter(
pk=OuterRef('pk'),
book__name=OuterRef('book__name'),
).values('pk')
author_qs = Author.objects.annotate(
subquery_id=Subquery(subquery_qs),
).annotate(count=Count('book'))
self.assertEqual(author_qs.count(), Author.objects.count())

def test_aggregation_order_by_not_selected_annotation_values(self):
result_asc = [
self.b4.pk,
Expand Down Expand Up @@ -1247,6 +1268,7 @@ def test_group_by_exists_annotation(self):
).annotate(total=Count('*'))
self.assertEqual(dict(has_long_books_breakdown), {True: 2, False: 3})

@skipUnlessDBFeature('supports_subqueries_in_group_by')
def test_aggregation_subquery_annotation_related_field(self):
publisher = Publisher.objects.create(name=self.a9.name, num_awards=2)
book = Book.objects.create(
Expand All @@ -1266,3 +1288,8 @@ def test_aggregation_subquery_annotation_related_field(self):
contact_publisher__isnull=False,
).annotate(count=Count('authors'))
self.assertSequenceEqual(books_qs, [book])
# FIXME: GROUP BY doesn't need to include a subquery with
# non-multivalued JOINs, see Col.possibly_multivalued (refs #31150):
# with self.assertNumQueries(1) as ctx:
# self.assertSequenceEqual(books_qs, [book])
# self.assertEqual(ctx[0]['sql'].count('SELECT'), 2)

0 comments on commit c5cfaad

Please sign in to comment.