#34798 closed Bug (fixed)
Subquery wrapping is required in QuerySet.aggregate() for aggregates referencing nested subquery.
Reported by: | Haldun Komsuoglu | Owned by: | Simon Charette |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 4.2 |
Severity: | Release blocker | Keywords: | MSSQL, Aggregation, Subquery |
Cc: | Simon Charette | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Aggregation in a query employing a subquery expression fails with the following error:
ProgrammingError: ('42000', '[42000] [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Cannot perform an aggregate function on an expression containing an aggregate or a subquery. (130) (SQLExecDirectW)')
However, the same query works in Django 4.1.10.
To generate the error for following example can be used:
class Exchange(models.Model): date = models.DateField() value = models.FloatField() class Invoice(models.Model): date = models.DateField() gross = models.FloatField() exchange = Subquery(Exchange.objects.filter(date__lte=OuterRef('date')).order_by('-date').values('value')[:1]) Invoice.objects.annotate( exchange=exchange, gross_currency=F('gross') / F('exchange') ).aggregate( avg_gross_currency=Avg('gross_currency') )
Change History (11)
comment:1 by , 15 months ago
Description: | modified (diff) |
---|
comment:2 by , 15 months ago
Description: | modified (diff) |
---|---|
Resolution: | → invalid |
Status: | new → closed |
comment:3 by , 15 months ago
This seems to be related to #34551 somehow. The refs_subquery
check added in e5c844d6f2a4ac6ae674d741b5f1fa2a688cedf4 is not transitive in the sense that it would work in cases where exchange
is referenced directly but not when not through combined expression. I'm surprised that the test added there doesn't fail if the aggregation is made through another F
annotation referring the subquery annotation.
comment:4 by , 15 months ago
Cc: | added |
---|
As an example the following patch
-
tests/aggregation/tests.py
diff --git a/tests/aggregation/tests.py b/tests/aggregation/tests.py index ad00afdcc1..63d446196e 100644
a b def test_referenced_subquery_requires_wrapping(self): 2249 2249 with self.assertNumQueries(1) as ctx: 2250 2250 aggregate = ( 2251 2251 Author.objects.annotate( 2252 total_books=Subquery(total_books_qs.values("total")) 2252 total_books=Subquery(total_books_qs.values("total")), 2253 total_books_ref=F("total_books") / 1, 2253 2254 ) 2254 .values("pk", "total_books ")2255 .values("pk", "total_books_ref") 2255 2256 .aggregate( 2256 sum_total_books=Sum("total_books "),2257 sum_total_books=Sum("total_books_ref"), 2257 2258 ) 2258 2259 ) 2259 2260 sql = ctx.captured_queries[0]["sql"].lower()
Makes the aggregation.tests.AggregateAnnotationPruningTests.test_referenced_subquery_requires_wrapping
test fail because the total_books_ref
annotation doesn't have a .subquery
property. What I think is required here is an Expression.returns_set -> bool
method (better name welcome) that is transitive in the sense that it behaves like contains_aggregate
does with regards to nested expressions.
This flag would also have other possible use cases but I wonder if it'd be too invasive for a backport.
Here's what it a backportable solution could look like
-
django/db/models/expressions.py
diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py index a3d08d4734..40a47c4873 100644
a b def contains_column_references(self): 256 256 for expr in self.get_source_expressions() 257 257 ) 258 258 259 @cached_property 260 def contains_subquery(self): 261 return ( 262 expr and expr.contains_subquery for expr in self.get_source_expressions() 263 ) 264 259 265 def resolve_expression( 260 266 self, query=None, allow_joins=True, reuse=None, summarize=False, for_save=False 261 267 ): … … class Subquery(BaseExpression, Combinable): 1544 1550 contains_aggregate = False 1545 1551 empty_result_set_value = None 1546 1552 subquery = True 1553 contains_subquery = True 1547 1554 1548 1555 def __init__(self, queryset, output_field=None, **extra): 1549 1556 # Allow the usage of both QuerySet and sql.Query objects. -
django/db/models/sql/query.py
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py index 9853919482..8013fcefe6 100644
a b class Query(BaseExpression): 189 189 190 190 filter_is_sticky = False 191 191 subquery = False 192 contains_subquery = True 192 193 193 194 # SQL-related attributes. 194 195 # Select and related select clauses are expressions to use in the SELECT … … def get_aggregation(self, using, aggregate_exprs): 420 421 # members of `aggregates` to resolve against each others. 421 422 self.append_annotation_mask([alias]) 422 423 refs_subquery |= any( 423 getattr(self.annotations[ref], " subquery", False)424 getattr(self.annotations[ref], "contains_subquery", False) 424 425 for ref in aggregate.get_refs() 425 426 ) 426 427 refs_window |= any(
comment:5 by , 13 months ago
Resolution: | invalid |
---|---|
Severity: | Normal → Release blocker |
Status: | closed → new |
Summary: | Using Django 4.2 with MSSQL 2019 Aggregation Containing Subquery Fails → Subquery wrapping is required in QuerySet.aggregate() for aggregates referencing nested subquery. |
Triage Stage: | Unreviewed → Accepted |
Simon, thanks for the extra details! As far as I'm aware it's a regression in 59bea9efd2768102fc9d3aedda469502c218e9b7.
comment:6 by , 13 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
follow-up: 8 comment:7 by , 13 months ago
Has patch: | set |
---|
Submitted a MR without release notes as I wasn't sure if we wanted to backport to 4.2. I assume so?
comment:8 by , 13 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
Replying to Simon Charette:
Submitted a MR without release notes as I wasn't sure if we wanted to backport to 4.2. I assume so?
Yes, I added release notes.
Thanks for the report, however I cannot reproduce it using any builtin backend. You should try to report it to the issue tracker of the 3rd-party database backend that you're using.