Opened 7 years ago

Closed 17 months 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

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

Download all attachments as: .zip

Change History (7)

Changed 7 years ago by Ghislain Leveque

This patch add support for the timedelta aggregation

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

Needs documentation: unset
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 7 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:

http://gist.github.com/333355

comment:3 Changed 5 years ago by Matt McClanahan

Severity: Normal
Type: Bug

comment:4 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:5 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:6 Changed 17 months 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