Opened 2 years ago

Closed 2 years ago

#31251 closed Cleanup/optimization (fixed)

QuerySet.annotate() crashes when grouping by OuterRef().

Reported by: Oliver Ford Owned by: Rohit Jha
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: db, outerref, group by, subquery, annotate
Cc: Rohit Jha Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

I'm trying to construct a query like:

cheapest_query = models.Subquery(qs.annotate(
    groupbytype=models.OuterRef("type"),
).values("groupbytype")).annotate(
    cheapest=models.Min("price"),
).values("cheapest"))

qs = qs.annotate(premium_over_cheapest=models.F("price") - cheapest_query)

However this fails in the second annotate (cheapest=) since ResolvedOuterRef does not descend from BaseExpression; so it has no get_group_by_cols method:

   File "/usr/local/lib/python3.8/site-packages/django/db/models/query.py", line 1078, in annotate
     clone.query.set_group_by()
  File "/usr/local/lib/python3.8/site-packages/django/db/models/sql/query.py", line 1938, in set_group_by
     inspect.getcallargs(annotation.get_group_by_cols, alias=alias)
 AttributeError: 'ResolvedOuterRef' object has no attribute 'get_group_by_cols'

https://github.com/django/django/blob/master/django/db/models/expressions.py

Change History (13)

comment:1 Changed 2 years ago by Oliver Ford

Perhaps I was doing something silly in the first place; I've resolved my issue by changing the subquery to first annotate (group by) all the types, whatever they are, not OuterRef, and then only after the Min annotation filtering on the OuterRef("type") matching the grouped by type.

I'll leave this open for now though in case it's desired to improve the error message in this case, or perhaps there's a more sensibly-intentioned query that does produce the same error.

comment:2 Changed 2 years ago by Mariusz Felisiak

Easy pickings: set
Summary: Unable to annotate a query that has an OuterRef alreadyQuerySet.annotate() crashes when grouping by OuterRef().
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization
Version: 3.0master

Grouping by OuterRef() doesn't have much value, it's like using a constant value, so IMO we can easily fix this with:

diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py
index 2e831b21ac..a165923784 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -562,6 +562,9 @@ class ResolvedOuterRef(F):
     def relabeled_clone(self, relabels):
         return self
 
+    def get_group_by_cols(self, alias=None):
+        return []
+
 
 class OuterRef(F):
     def resolve_expression(self, *args, **kwargs):

comment:3 Changed 2 years ago by Yuri Savin

Owner: changed from nobody to Yuri Savin
Status: newassigned

comment:4 Changed 2 years ago by Oliver Ford

Why not the column that OuterRef is referencing? That was my intended behaviour, though I eventually realised I was overcomplicating it and just needed to group on the column directly, not an outer ref, and filter on the outer ref.

comment:5 Changed 2 years ago by Mariusz Felisiak

Oliver, OuterRef() is a field from the outer query, so it will be the same for all rows in a subquery. Is there any value in adding it to a GROUP BY clause? e.g.

(SELECT MIN("price") from "subquery_table" GROUP BY "outer_table"."type")

is equivalent to

(SELECT MIN("price") from "subquery_table")

comment:6 Changed 2 years ago by Yuri Savin

Owner: Yuri Savin deleted
Status: assignednew

comment:7 in reply to:  5 Changed 2 years ago by Yuri Savin

I think I overestimated my strength with this task

Last edited 2 years ago by Yuri Savin (previous) (diff)

comment:8 Changed 2 years ago by Rohit Jha

Owner: set to Rohit Jha
Status: newassigned

comment:9 Changed 2 years ago by Rohit Jha

Has patch: set
Last edited 2 years ago by Rohit Jha (previous) (diff)

comment:10 Changed 2 years ago by Simon Charette

Needs tests: set
Patch needs improvement: set

comment:11 Changed 2 years ago by Rohit Jha

Cc: Rohit Jha added
Needs tests: unset

comment:12 Changed 2 years ago by Mariusz Felisiak

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:13 Changed 2 years ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 486786c:

Fixed #31251 -- Disabled grouping by OuterRef() annotation.

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