Opened 13 years ago

Closed 7 years ago

#12471 closed Bug (fixed)

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

Reported by: Ghislain Leveque 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


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 Ghislain Leveque 13 years ago.
This patch add support for the timedelta aggregation

Download all attachments as: .zip

Change History (7)

Changed 13 years ago by Ghislain Leveque

This patch add support for the timedelta aggregation

comment:1 Changed 13 years ago by Russell Keith-Magee

Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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 12 years ago by Collin Anderson

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:

comment:3 Changed 11 years ago by Matt McClanahan

Severity: Normal
Type: Bug

comment:4 Changed 10 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:5 Changed 10 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:6 Changed 7 years ago by Tim Graham

Resolution: fixed
Status: newclosed

The code in question in gone as of Django 1.8 and the expressions refactor.

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