From 5f09ab8c30050bbd076a9b27fb135d030c06ab75 Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Sat, 5 Nov 2022 12:49:33 -0400 Subject: [PATCH] Refs #17144 -- Removed support for grouping by primary key. No core backend require the feature anymore as it was only added to support a MySQL'ism that has been deprecated since then. --- django/db/backends/base/features.py | 1 - django/db/models/sql/compiler.py | 39 +++-------------------------- docs/releases/4.2.txt | 7 +++++- tests/aggregation_regress/tests.py | 14 ++++------- tests/annotations/tests.py | 15 ----------- 5 files changed, 15 insertions(+), 61 deletions(-) diff --git a/django/db/backends/base/features.py b/django/db/backends/base/features.py index 6b935083836..a1d38d3530f 100644 --- a/django/db/backends/base/features.py +++ b/django/db/backends/base/features.py @@ -8,7 +8,6 @@ class BaseDatabaseFeatures: gis_enabled = False # Oracle can't group by LOB (large object) data types. allows_group_by_lob = True - allows_group_by_pk = False allows_group_by_selected_pks = False allows_group_by_refs = True empty_fetchmany_value = [] diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index b6574eab2e9..97c7ba2013a 100644 --- a/django/db/models/sql/compiler.py +++ b/django/db/models/sql/compiler.py @@ -179,41 +179,10 @@ class SQLCompiler: return result def collapse_group_by(self, expressions, having): - # If the DB can group by primary key, then group by the primary key of - # query's main model. Note that for PostgreSQL the GROUP BY clause must - # include the primary key of every table, but for MySQL it is enough to - # have the main table's primary key. - if self.connection.features.allows_group_by_pk: - # Determine if the main model's primary key is in the query. - pk = None - for expr in expressions: - # Is this a reference to query's base table primary key? If the - # expression isn't a Col-like, then skip the expression. - if ( - getattr(expr, "target", None) == self.query.model._meta.pk - and getattr(expr, "alias", None) == self.query.base_table - ): - pk = expr - break - # If the main model's primary key is in the query, group by that - # field, HAVING expressions, and expressions associated with tables - # that don't have a primary key included in the grouped columns. - if pk: - pk_aliases = { - expr.alias - for expr in expressions - if hasattr(expr, "target") and expr.target.primary_key - } - expressions = [pk] + [ - expr - for expr in expressions - if expr in having - or ( - getattr(expr, "alias", None) is not None - and expr.alias not in pk_aliases - ) - ] - elif self.connection.features.allows_group_by_selected_pks: + # If the database supports group by functional dependence reduction, + # then the expressions can be reduced to the set of selected table + # primary keys as all other columns are functionally dependent on them. + if self.connection.features.allows_group_by_selected_pks: # Filter out all expressions associated with a table's primary key # present in the grouped columns. This is done by identifying all # tables that have their primary key included in the grouped diff --git a/docs/releases/4.2.txt b/docs/releases/4.2.txt index ba01bf12e5a..bf0a36fce61 100644 --- a/docs/releases/4.2.txt +++ b/docs/releases/4.2.txt @@ -309,7 +309,12 @@ Database backend API This section describes changes that may be needed in third-party database backends. -* ... +* ``DatabaseFeatures.allows_group_by_pk`` is removed as it only remained to + accommodate a MySQL extension that has been supplanted by proper functional + dependency detection in MySQL 5.7.15. Note that + ``DatabaseFeatures.allows_group_by_selected_pks`` is still supported and + should be enabled if your backend supports functional dependency detection in + ``GROUP BY`` clauses as specified by the ``SQL:1999`` standard. Dropped support for MariaDB 10.3 -------------------------------- diff --git a/tests/aggregation_regress/tests.py b/tests/aggregation_regress/tests.py index d3d06fe076b..444a55276d3 100644 --- a/tests/aggregation_regress/tests.py +++ b/tests/aggregation_regress/tests.py @@ -23,7 +23,7 @@ from django.db.models import ( Variance, When, ) -from django.test import TestCase, skipUnlessAnyDBFeature, skipUnlessDBFeature +from django.test import TestCase, skipUnlessDBFeature from django.test.utils import Approximate from .models import ( @@ -1420,7 +1420,7 @@ class AggregationTests(TestCase): # The query executes without problems. self.assertEqual(len(qs.exclude(publisher=-1)), 6) - @skipUnlessAnyDBFeature("allows_group_by_pk", "allows_group_by_selected_pks") + @skipUnlessDBFeature("allows_group_by_selected_pks") def test_aggregate_duplicate_columns(self): # Regression test for #17144 @@ -1448,7 +1448,7 @@ class AggregationTests(TestCase): ], ) - @skipUnlessAnyDBFeature("allows_group_by_pk", "allows_group_by_selected_pks") + @skipUnlessDBFeature("allows_group_by_selected_pks") def test_aggregate_duplicate_columns_only(self): # Works with only() too. results = Author.objects.only("id", "name").annotate( @@ -1474,18 +1474,14 @@ class AggregationTests(TestCase): ], ) - @skipUnlessAnyDBFeature("allows_group_by_pk", "allows_group_by_selected_pks") + @skipUnlessDBFeature("allows_group_by_selected_pks") def test_aggregate_duplicate_columns_select_related(self): # And select_related() results = Book.objects.select_related("contact").annotate( num_authors=Count("authors") ) _, _, grouping = results.query.get_compiler(using="default").pre_sql_setup() - # In the case of `group_by_selected_pks` we also group by contact.id - # because of the select_related. - self.assertEqual( - len(grouping), 1 if connection.features.allows_group_by_pk else 2 - ) + self.assertEqual(len(grouping), 2) self.assertIn("id", grouping[0][0]) self.assertNotIn("name", grouping[0][0]) self.assertNotIn("contact", grouping[0][0]) diff --git a/tests/annotations/tests.py b/tests/annotations/tests.py index 472669288c6..52c15bba873 100644 --- a/tests/annotations/tests.py +++ b/tests/annotations/tests.py @@ -550,21 +550,6 @@ class NonAggregateAnnotationTestCase(TestCase): for publisher in publishers.filter(pk=self.p1.pk): self.assertEqual(publisher["book__rating"], publisher["total"]) - @skipUnlessDBFeature("allows_group_by_pk") - def test_rawsql_group_by_collapse(self): - raw = RawSQL("SELECT MIN(id) FROM annotations_book", []) - qs = ( - Author.objects.values("id") - .annotate( - min_book_id=raw, - count_friends=Count("friends"), - ) - .order_by() - ) - _, _, group_by = qs.query.get_compiler(using="default").pre_sql_setup() - self.assertEqual(len(group_by), 1) - self.assertNotEqual(raw, group_by[0]) - def test_defer_annotation(self): """ Deferred attributes can be referenced by an annotation,