Code

Opened 3 years ago

Closed 9 months ago

#15624 closed Bug (fixed)

annotate + aggregate produces incorrect sql statement

Reported by: zeroos <232002@…> Owned by: fhahn
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: aggregate sql syntax error
Cc: flo@…, jmilner 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

I have a following model:

class Solution(models.Model): 
    griddler = models.ForeignKey(Griddler) 
    user = models.ForeignKey(User) 
    user_time = models.IntegerField(null=True) 
    date = models.DateTimeField(auto_now_add=True) 
class Griddler(models.Model): 
    .... 
    solved_by = models.ManyToManyField(Uesr, through='Solution') 
    .... 

What I am trying to do is to find the Griddler that has the biggest
avg user time. I've written the following query:
Solution.objects.values('griddler').annotate(a=Avg('user_time')).aggregate( Max('a'))

Unfortunately, after executing it I get

DatabaseError: (1064, "You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'FROM (SELECT griddlers_solution.griddler_id AS griddler_id, AVG(`griddlers' at line 1")

>>> connection.queries[-1] 
{'time': '0.000', 'sql': u'SELECT  FROM (SELECT 
`griddlers_solution`.`griddler_id` AS `griddler_id`, 
AVG(`griddlers_solution`.`user_time`) AS `a` FROM `griddlers_solution` 
GROUP BY `griddlers_solution`.`griddler_id`, 
`griddlers_solution`.`griddler_id` ORDER BY NULL) subquery'} 

The problem is that Max('a') is skipped in the sql query. Instead of :

SELECT  FROM (SELECT ...

It should look like this:

SELECT Max(`a`) FROM (SELECT... 

I am using Django from SVN (pulled today, 16.03.11) and MySQL backend.

>>> django.get_version() 
'1.3 beta 1'

Attachments (3)

15624.test.diff (938 bytes) - added by mk 3 years ago.
aggregate_bug_quick_patch.diff (869 bytes) - added by marekw2143 3 years ago.
That's a quick hack that should work, but there probably exist more elegant solution.
aggregate_bug_quick_patch2.diff (1.4 KB) - added by mwawrzyczek 3 years ago.
Added more consistent version of the patch.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 3 years ago by anonymous

  • Component changed from Database layer (models, ORM) to ORM aggregation
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 3 years ago by marekw2143

  • Owner changed from nobody to marekw2143
  • Status changed from new to assigned

comment:3 Changed 3 years ago by mk

  • Triage Stage changed from Unreviewed to Accepted

Attached patch with a unit test confirming this issue.

Changed 3 years ago by mk

comment:4 Changed 3 years ago by lukeplant

  • Type set to Bug

comment:5 Changed 3 years ago by lukeplant

  • Severity set to Normal

Changed 3 years ago by marekw2143

That's a quick hack that should work, but there probably exist more elegant solution.

Changed 3 years ago by mwawrzyczek

Added more consistent version of the patch.

comment:6 Changed 3 years ago by ramiro

  • Easy pickings unset
  • Has patch set
  • Needs tests set
  • UI/UX unset

comment:7 Changed 2 years ago by marekw2143

  • Owner marekw2143 deleted
  • Status changed from assigned to new

comment:8 Changed 15 months ago by stefano@…

Hello! Any news on that? I see there's a patch, now oldish, but nothing happened since and nobody from core has commented on it... is that patch a viable solution?

comment:9 Changed 14 months ago by fhahn

  • Cc flo@… added
  • Needs tests unset
  • Owner set to fhahn
  • Status changed from new to assigned

I think the problem with Book.objects.annotate(c=Count('authors')).values('c').aggregate(Max('c')) was that the call to .values('c') set the aggregates_select_mask to set(['c']) and then c__max does not show up in aggregate_select .

My patch adds the alias of the aggregate to aggregate_select_mask , if aggregate_select_mask is not None.

I've updated the patch and created a pull request: https://github.com/django/django/pull/738

Last edited 14 months ago by fhahn (previous) (diff)

comment:10 Changed 14 months ago by akaariai

  • Component changed from ORM aggregation to Database layer (models, ORM)

comment:11 Changed 11 months ago by jmilner

  • Cc jmilner added

comment:12 Changed 9 months ago by timo

  • Summary changed from aggregate produces incorrect sql statement to annotate + aggregate produces incorrect sql statement
  • Triage Stage changed from Accepted to Ready for checkin

Patch applies cleanly and tests pass. Would like someone more familiar with the code to +1 before committing.

comment:13 Changed 9 months ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In e888a9b30dd9d62a930b9244d1b5531bb17544b4:

Fixed #15624 -- Made sure aggregations are present in SELECT

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.