Code

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#10962 closed (invalid)

GROUP BY generation and extra seems to be too greedy

Reported by: brosner Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I may not understand the ORM code fully, but this seems like wrong behavior. Take the following queryset:

queryset = channel.message_set.all().values(
   "nickname",
).annotate(
   message_count = models.Count("id"),
).extra(select={
    "percentage": "FLOOR((COUNT(*) / %d.0) * 100)" % channel.message_set.count(),
}).order_by("-message_count")

I am seeing the following query generated:

SELECT (FLOOR((COUNT(*) / 1105908.0) * 100)) AS "percentage",
       "irc_message"."nickname", COUNT("irc_message"."id") AS "message_count"
FROM "irc_message"
WHERE "irc_message"."channel_id" = 3
GROUP BY "irc_message"."nickname",
         FLOOR((COUNT(*) / 1105908.0) * 100)
ORDER BY message_count DESC LIMIT 10

As you can see the expression from extra(select=) is sneaking into the GROUP BY clause. Tracking this down led to #10132 which seems to introduce this new behavior (even though I don't know if things worked before [9838]).

Attachments (0)

Change History (3)

comment:1 Changed 5 years ago by brosner

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

After thinking about this a bit more it does appear to be doing the most correct thing. My specific use-case was able to do done via a custom aggregate. I'll leave open just in-case there was something overlooked.

comment:2 Changed 5 years ago by russellm

  • Resolution set to invalid
  • Status changed from new to closed

Your thinking is correct. Any extra(select=) columns are assumed to be non-aggregates, and are therefore included in the GROUP BY; it is assumed that if you need an aggregate in the extra, you can use annotate() and a custom aggregate.

comment:3 Changed 3 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.