Fixed #31094 -- Included columns referenced by subqueries in GROUP BY on aggregations.

Thanks Johannes Hoppe for the report.

Regression in fb3f034f1c.

Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com>
This commit is contained in:
Simon Charette 2019-12-17 01:21:13 -05:00 committed by Mariusz Felisiak
parent a0f34d8fef
commit 5a4d7285bd
5 changed files with 58 additions and 18 deletions

View File

@ -1040,7 +1040,7 @@ class Subquery(Expression):
def get_group_by_cols(self, alias=None): def get_group_by_cols(self, alias=None):
if alias: if alias:
return [Ref(alias, self)] return [Ref(alias, self)]
return [] return self.query.get_external_cols()
class Exists(Subquery): class Exists(Subquery):

View File

@ -109,7 +109,14 @@ class SQLCompiler:
# Note that even if the group_by is set, it is only the minimal # Note that even if the group_by is set, it is only the minimal
# set to group by. So, we need to add cols in select, order_by, and # set to group by. So, we need to add cols in select, order_by, and
# having into the select in any case. # having into the select in any case.
ref_sources = {
expr.source for expr in expressions if isinstance(expr, Ref)
}
for expr, _, _ in select: for expr, _, _ in select:
# Skip members of the select clause that are already included
# by reference.
if expr in ref_sources:
continue
cols = expr.get_group_by_cols() cols = expr.get_group_by_cols()
for col in cols: for col in cols:
expressions.append(col) expressions.append(col)
@ -399,7 +406,7 @@ class SQLCompiler:
return self.quote_cache[name] return self.quote_cache[name]
if ((name in self.query.alias_map and name not in self.query.table_map) or if ((name in self.query.alias_map and name not in self.query.table_map) or
name in self.query.extra_select or ( name in self.query.extra_select or (
name in self.query.external_aliases and name not in self.query.table_map)): self.query.external_aliases.get(name) and name not in self.query.table_map)):
self.quote_cache[name] = name self.quote_cache[name] = name
return name return name
r = self.connection.ops.quote_name(name) r = self.connection.ops.quote_name(name)

View File

@ -157,7 +157,8 @@ class Query(BaseExpression):
# Sometimes the query contains references to aliases in outer queries (as # Sometimes the query contains references to aliases in outer queries (as
# a result of split_exclude). Correct alias quoting needs to know these # a result of split_exclude). Correct alias quoting needs to know these
# aliases too. # aliases too.
self.external_aliases = set() # Map external tables to whether they are aliased.
self.external_aliases = {}
self.table_map = {} # Maps table names to list of aliases. self.table_map = {} # Maps table names to list of aliases.
self.default_cols = True self.default_cols = True
self.default_ordering = True self.default_ordering = True
@ -855,8 +856,11 @@ class Query(BaseExpression):
if alias == old_alias: if alias == old_alias:
table_aliases[pos] = new_alias table_aliases[pos] = new_alias
break break
self.external_aliases = {change_map.get(alias, alias) self.external_aliases = {
for alias in self.external_aliases} # Table is aliased or it's being changed and thus is aliased.
change_map.get(alias, alias): (aliased or alias in change_map)
for alias, aliased in self.external_aliases.items()
}
def bump_prefix(self, outer_query): def bump_prefix(self, outer_query):
""" """
@ -1030,19 +1034,23 @@ class Query(BaseExpression):
for key, value in clone.annotations.items(): for key, value in clone.annotations.items():
resolved = value.resolve_expression(query, *args, **kwargs) resolved = value.resolve_expression(query, *args, **kwargs)
if hasattr(resolved, 'external_aliases'): if hasattr(resolved, 'external_aliases'):
resolved.external_aliases.update(clone.alias_map) resolved.external_aliases.update(clone.external_aliases)
clone.annotations[key] = resolved clone.annotations[key] = resolved
# Outer query's aliases are considered external. # Outer query's aliases are considered external.
clone.external_aliases.update( for alias, table in query.alias_map.items():
alias for alias, table in query.alias_map.items() clone.external_aliases[alias] = (
if ( (isinstance(table, Join) and table.join_field.related_model._meta.db_table != alias) or
isinstance(table, Join) and table.join_field.related_model._meta.db_table != alias (isinstance(table, BaseTable) and table.table_name != table.table_alias)
) or (
isinstance(table, BaseTable) and table.table_name != table.table_alias
) )
)
return clone return clone
def get_external_cols(self):
exprs = chain(self.annotations.values(), self.where.children)
return [
col for col in self._gen_cols(exprs)
if col.alias in self.external_aliases
]
def as_sql(self, compiler, connection): def as_sql(self, compiler, connection):
sql, params = self.get_compiler(connection=connection).as_sql() sql, params = self.get_compiler(connection=connection).as_sql()
if self.subquery: if self.subquery:
@ -1635,12 +1643,16 @@ class Query(BaseExpression):
return targets, joins[-1], joins return targets, joins[-1], joins
@classmethod @classmethod
def _gen_col_aliases(cls, exprs): def _gen_cols(cls, exprs):
for expr in exprs: for expr in exprs:
if isinstance(expr, Col): if isinstance(expr, Col):
yield expr.alias yield expr
else: else:
yield from cls._gen_col_aliases(expr.get_source_expressions()) yield from cls._gen_cols(expr.get_source_expressions())
@classmethod
def _gen_col_aliases(cls, exprs):
yield from (expr.alias for expr in cls._gen_cols(exprs))
def resolve_ref(self, name, allow_joins=True, reuse=None, summarize=False): def resolve_ref(self, name, allow_joins=True, reuse=None, summarize=False):
if not allow_joins and LOOKUP_SEP in name: if not allow_joins and LOOKUP_SEP in name:
@ -1733,7 +1745,7 @@ class Query(BaseExpression):
lookup = lookup_class(pk.get_col(query.select[0].alias), lookup = lookup_class(pk.get_col(query.select[0].alias),
pk.get_col(alias)) pk.get_col(alias))
query.where.add(lookup, AND) query.where.add(lookup, AND)
query.external_aliases.add(alias) query.external_aliases[alias] = True
condition, needed_inner = self.build_filter( condition, needed_inner = self.build_filter(
('%s__in' % trimmed_prefix, query), ('%s__in' % trimmed_prefix, query),

View File

@ -9,4 +9,5 @@ Django 3.0.2 fixes several bugs in 3.0.1.
Bugfixes Bugfixes
======== ========
* ... * Fixed a regression in Django 3.0 that didn't include columns referenced by a
``Subquery()`` in the ``GROUP BY`` clause (:ticket:`31094`).

View File

@ -1172,3 +1172,23 @@ class AggregateTestCase(TestCase):
Exists(long_books_qs), Exists(long_books_qs),
).annotate(total=Count('*')) ).annotate(total=Count('*'))
self.assertEqual(dict(has_long_books_breakdown), {True: 2, False: 3}) self.assertEqual(dict(has_long_books_breakdown), {True: 2, False: 3})
def test_aggregation_subquery_annotation_related_field(self):
publisher = Publisher.objects.create(name=self.a9.name, num_awards=2)
book = Book.objects.create(
isbn='159059999', name='Test book.', pages=819, rating=2.5,
price=Decimal('14.44'), contact=self.a9, publisher=publisher,
pubdate=datetime.date(2019, 12, 6),
)
book.authors.add(self.a5, self.a6, self.a7)
books_qs = Book.objects.annotate(
contact_publisher=Subquery(
Publisher.objects.filter(
pk=OuterRef('publisher'),
name=OuterRef('contact__name'),
).values('name')[:1],
)
).filter(
contact_publisher__isnull=False,
).annotate(count=Count('authors'))
self.assertSequenceEqual(books_qs, [book])