Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#14548 closed Bug (duplicate)

Docs and implementation don't match regading return values of certain QuerySet aggregate functions

Reported by: PaulM Owned by: nobody
Component: Documentation Version: 1.2
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX:

Description

The aggregation docs specify that Max, Min, or Sum return a value of the same type as the field on which the function was called. This means that if I use Max() on a PositiveIntegerField, I expect to get back a Python int.

I know that a non-null PositiveIntegerField can't contain None - setting and saving it throws an IntegrityError.

However, when I use the Max() aggregate on a PositiveIntegerField in a model with no rows, I get back None. This behavior occurs in both sqlite and PostgreSQL.

Since this behavior has (presumably) been around since aggregates were introduced, this must be a bug in the docs, and not in Django, even though it is counterintuitive and requires boilerplate code to make absolutely certain that the value is not None before depending on the typing.

If it were up to me, I would propose changing the return value to be in line with the docs, but that wouldn't be backwards compatible.

Change History (10)

comment:1 Changed 4 years ago by Alex

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

I'd be -1 on changing the implementation, there's no obvious return value for any of those on no values (with the exception of Sum).

comment:2 Changed 4 years ago by PaulM

What is obnoxious is having to do something like:

MyModel.objects.aggregate(Max('my_integer_field')) or 0

That's required everywhere I use Max as a number, and the conditional only matters in the corner case of my DB having no data in it. Having "or 0 + 1" all over the place looks pretty ugly.

I agree that good return values instead of None aren't obvious for all data types. I feel that Max and Min of an int should be zero, though, for the same reason as Sum. If consistency among these functions is more important to us here than real world usage, the docs should have a big warning about this.

comment:3 Changed 4 years ago by gabrielhurley

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 4 years ago by lukeplant

I really can't agree that zero is a good default for either Max or Min (though you could make a case for 0 being a good default Min value for *positive* integer fields).
The current behaviour might be for annoying your use case, but in general, an exception caused by 'None' is exactly what you want to occur, since it will often be a logic error to attempt to do anything with the Max or Min of a non-existent set of data. Errors like that should not be silenced implicitly.

BTW - Min and Max are completely different from Sum. It makes sense for Sum to have a zero default value, because the following holds:

 0 + x1 + x2 + ... == x1 + x2 + ...

So you can express sum as a 'fold' with a zero starting value.

But the same does not hold for the min or max 'operators' (using Haskell syntax):

 0 `min` x1 `min` x2 ... /=  x1 `min` x2 ...

 0 `max` x1 `max` x2 ... /=  x1 `max` x2 ...

comment:5 Changed 4 years ago by Alex

Perhaps there is a case for aggregates to take a default argument though.

comment:6 Changed 4 years ago by gabrielhurley

I'd be +1 on a default argument for aggregates.

For an extra win, documenting the new default argument's default value(s) would also solve the current documentation problem! Can we reclassify this ticket as "ORM Aggregation" and call it a feature request, then?

comment:7 Changed 4 years ago by ramiro

  • Summary changed from Aggregates docs are misleading (or there is a bug in the aggregate code) to Docs and implementation don't match regading return values of certain QuerySet aggregate functions

(Changing summary to something more concrete so I don't need to click and read the ticket every time I see it in a report to know what it's about.)

comment:8 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:9 Changed 4 years ago by ramiro

  • Easy pickings unset
  • Resolution set to duplicate
  • Status changed from new to closed

Following favorable opinions about implementing a default value argument I'm going to colse this ticket as duplicate of #10929.

comment:10 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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