Opened 15 months ago

Closed 13 months ago

Last modified 13 months ago

#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 Mariusz Felisiak)

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 Haldun Komsuoglu, 15 months ago

Description: modified (diff)

comment:2 by Mariusz Felisiak, 15 months ago

Description: modified (diff)
Resolution: invalid
Status: newclosed

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.

comment:3 by Simon Charette, 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 Simon Charette, 15 months ago

Cc: Simon Charette 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):  
    22492249        with self.assertNumQueries(1) as ctx:
    22502250            aggregate = (
    22512251                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,
    22532254                )
    2254                 .values("pk", "total_books")
     2255                .values("pk", "total_books_ref")
    22552256                .aggregate(
    2256                     sum_total_books=Sum("total_books"),
     2257                    sum_total_books=Sum("total_books_ref"),
    22572258                )
    22582259            )
    22592260        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):  
    256256            for expr in self.get_source_expressions()
    257257        )
    258258
     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
    259265    def resolve_expression(
    260266        self, query=None, allow_joins=True, reuse=None, summarize=False, for_save=False
    261267    ):
    class Subquery(BaseExpression, Combinable):  
    15441550    contains_aggregate = False
    15451551    empty_result_set_value = None
    15461552    subquery = True
     1553    contains_subquery = True
    15471554
    15481555    def __init__(self, queryset, output_field=None, **extra):
    15491556        # 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):  
    189189
    190190    filter_is_sticky = False
    191191    subquery = False
     192    contains_subquery = True
    192193
    193194    # SQL-related attributes.
    194195    # Select and related select clauses are expressions to use in the SELECT
    def get_aggregation(self, using, aggregate_exprs):  
    420421            # members of `aggregates` to resolve against each others.
    421422            self.append_annotation_mask([alias])
    422423            refs_subquery |= any(
    423                 getattr(self.annotations[ref], "subquery", False)
     424                getattr(self.annotations[ref], "contains_subquery", False)
    424425                for ref in aggregate.get_refs()
    425426            )
    426427            refs_window |= any(

comment:5 by Mariusz Felisiak, 13 months ago

Resolution: invalid
Severity: NormalRelease blocker
Status: closednew
Summary: Using Django 4.2 with MSSQL 2019 Aggregation Containing Subquery FailsSubquery wrapping is required in QuerySet.aggregate() for aggregates referencing nested subquery.
Triage Stage: UnreviewedAccepted

Simon, thanks for the extra details! As far as I'm aware it's a regression in 59bea9efd2768102fc9d3aedda469502c218e9b7.

comment:6 by Simon Charette, 13 months ago

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:7 by Simon Charette, 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?

in reply to:  7 comment:8 by Mariusz Felisiak, 13 months ago

Triage Stage: AcceptedReady 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.

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 13 months ago

Resolution: fixed
Status: assignedclosed

In 3b4a5712:

Fixed #34798 -- Fixed QuerySet.aggregate() crash when referencing expressions containing subqueries.

Regression in 59bea9efd2768102fc9d3aedda469502c218e9b7,
complements e5c844d6f2a4ac6ae674d741b5f1fa2a688cedf4.

Refs #28477, #34551.

Thanks Haldun Komsuoglu for the report.

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 13 months ago

In 4ccca9ee:

[5.0.x] Fixed #34798 -- Fixed QuerySet.aggregate() crash when referencing expressions containing subqueries.

Regression in 59bea9efd2768102fc9d3aedda469502c218e9b7,
complements e5c844d6f2a4ac6ae674d741b5f1fa2a688cedf4.

Refs #28477, #34551.

Thanks Haldun Komsuoglu for the report.

Backport of 3b4a571275d967512866012955eb0b3ae486d63c from main

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 13 months ago

In 803caec:

[4.2.x] Fixed #34798 -- Fixed QuerySet.aggregate() crash when referencing expressions containing subqueries.

Regression in 59bea9efd2768102fc9d3aedda469502c218e9b7,
complements e5c844d6f2a4ac6ae674d741b5f1fa2a688cedf4.

Refs #28477, #34551.

Thanks Haldun Komsuoglu for the report.

Backport of 3b4a571275d967512866012955eb0b3ae486d63c from main

Note: See TracTickets for help on using tickets.
Back to Top