Code

Opened 4 years ago

Last modified 3 years ago

#12471 new Bug

Wrong assertion in resolve_aggregate in django.db.models.sql.query

Reported by: courtjus Owned by: nobody
Component: Database layer (models, ORM) Version: 1.1
Severity: Normal Keywords: aggregation
Cc: cmawebsite@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In the resolve_aggregate method in the django.db.models.sql.query module :

297 	        elif aggregate.is_computed:
298 	            # Any computed aggregate (e.g., avg) returns a float
299 	            return float(value)

The assertion that "any computed aggregate like AVG returns a float" is wrong. In particular, I use Postgresql/pyscopg2 and postgresql returns an interval (datetime.timedelta from the python point of vue) when the aggregated column is of type interval.

I made a quick and dirty patch attached to the ticket

Attachments (1)

support_timedelta_aggregate.patch (762 bytes) - added by courtjus 4 years ago.
This patch add support for the timedelta aggregation

Download all attachments as: .zip

Change History (6)

Changed 4 years ago by courtjus

This patch add support for the timedelta aggregation

comment:1 Changed 4 years ago by russellm

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

I'll mark this as accepted, but:

  1. I'm not a fan of the isinstance call. Unless you can prove that timedelta is the *only* type that needs this sort of raw handling, then your patch should be trying to identify the underlying condition that means that a raw database value should be passed through unmodified.
  2. I'd be very surprised if timedeltas are handled consistently across databases. Time handling varies wildly between backends. However, it's impossible to validate if this is a problem this because...
  3. Your patch doesn't contain tests. No patch is complete until there are tests to validate the new behavior.

comment:2 Changed 4 years ago by CollinAnderson

  • Cc cmawebsite@… added

Here is a similar issue with resolve_aggregate, although not 100% related. I would like to be able to annotate by a CharField instead of an aggregate. Currently I need to monkey patch resolve_aggregate on the fly to get it to return unicode instead of float. Please see my example code here:

http://gist.github.com/333355

comment:3 Changed 3 years ago by mattmcc

  • Severity set to Normal
  • Type set to Bug

comment:4 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:5 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.