Opened 4 years ago

Closed 4 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 by Oliver Ford, 4 years ago

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 by Mariusz Felisiak, 4 years ago

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 by Yuri Savin, 4 years ago

Owner: changed from nobody to Yuri Savin
Status: newassigned

comment:4 by Oliver Ford, 4 years ago

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 by Mariusz Felisiak, 4 years ago

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 by Yuri Savin, 4 years ago

Owner: Yuri Savin removed
Status: assignednew

in reply to:  5 comment:7 by Yuri Savin, 4 years ago

I think I overestimated my strength with this task

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

comment:8 by Rohit Jha, 4 years ago

Owner: set to Rohit Jha
Status: newassigned

comment:9 by Rohit Jha, 4 years ago

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

comment:10 by Simon Charette, 4 years ago

Needs tests: set
Patch needs improvement: set

comment:11 by Rohit Jha, 4 years ago

Cc: Rohit Jha added
Needs tests: unset

comment:12 by Mariusz Felisiak, 4 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 486786c:

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

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