Support a default value for Sum (and possibly other aggregation functions)

[nolan]

By default, annotate(sum_field = Sum(...)) results in sum_field being NULL if there were no values to sum. In most cases, 0 would be a better option here, for proper sorting in a later order_by, and for comparisons using lt/gt/gte/lte.

A monkeypatch to implement default values for NULL using COALESCE is available here:
​http://stackoverflow.com/questions/553038/treat-null-as-0-in-django-model

[Russell Keith-Magee]

This is a reasonable suggestion; variants in SQL syntax notwithstanding, it shouldn't be too hard to implement.

For those following the advice of Stack Overflow: There is no need to monkeypatch the sql_aggregates module - if you override the add_to_query function(), you can load the sql-specific aggregate from wherever you want.

In fact, you don't even need to have the split between generic and SQL specific aggregate. If all you want is a custom SQL aggregate that you can use in your own code, the following definition will help:

from django.db.models.sql.aggregates import Aggregate 
class MyAggregate(Aggregate): 
    sql_function = 'SOME_SQL_FUNC' 
    sql_template = 'SQL_TEMPLATE_BITS(%(function)s(%(field)s), %(default)s)'

    def __init__(self, lookup, **extra): 
        self.lookup = lookup 
        self.extra = extra 
    def _default_alias(self): 
        return '%s__%s' % (self.lookup, self.__class__.__name__.lower()) 
    default_alias = property(_default_alias) 
    def add_to_query(self, query, alias, col, source, is_summary): 
        super(MyAggregate, self).__init__(col, source, is_summary, **self.extra) 
        query.aggregate_select[alias] = self 

[Ramiro Morales]

#14548 was a duplicate, had some useful discussion in its comments.

[Tai Lee]

Just added a patch with an initial implementation for this feature. It should work for all aggregates by wrapping the default sql_template in COALESCE(%s, %%(default)s), if params has a default item. There's a basic test in there that checks the behaviour for Avg with and without a default. If there's positive feedback about the implementation, I'll have a go at adding documentation and any improvements to the implementation and tests that are suggested.

[Luke Plant]

There is a good case for giving Sum a default of zero, while leaving Max and Min with default of None. Essentially, the reason is that zero is the fixed point of the + operator, while max and min have no fixed points (excluding negative infinity and positive infinity respectively, which are problematic). Also, it makes the behaviour analogous to the Python builtins sum, min and max - sum returns 0 for an empty list, whereas min and max throw exceptions. (We don't want to throw exceptions here, for various obvious reasons, but I think we should be indicating 'undefined' in some way under the same circumstances).

If we do this, we need a note in the release notes about the backwards incompatibility - if people were relying on Sum returning None for no data instead of zero. (We can certainly classify the previous behaviour as a bug, since it was out of line with our docs, and neither expected or useful behaviour, but should still mention this). We should also have some tests that ensure that it returns a zero of the right type for different underlying field types.

This also brings up the possibility of whether the default for Sum should act like the start parameter of the sum Python builtin. I think it should. That would make implementation harder, though, I guess. Alternatively we could have a separate 'start' parameter for Sum, which might be clearer, and would make that a separate feature.

[Josh Smeaton]

I would support closing this as a duplicate of #23753. Once the linked ticket is implemented, it'll be possible for users to construct their own coalesce value like:

annotate(sum_field = Coalesce(Sum(...), 0))

This leaves the default behaviour alone, but provides a higher level of customisation. It relies on the #14030 patch which will also allow adding a "start" value, as mentioned by @lukeplant above:

annotate(sum_field = Coalesce(Sum(...), 0) + Value(5))

Would everyone be happy enough with the above use?

[Tai Lee]

I'm not totally convinced (yet) that we should give up on this when Coalesce() becomes available. If we just instruct people to use Coalesce(Sum(...), default) all the time, we're basically just recreating the literal semantics of SQL itself rather than providing an easy to use abstraction of it. Sure, it would be useful and powerful to have Coalesce() as well, but we might not need to force people to use it all the time for the common case. At the very least, a decision on closing this ticket should probably wait until Coalesce() is actually implemented.

[Josh Smeaton]

@mrmachine that's not a bad argument to make. What we could support is:

Sum('field', default=0)

Which would then be returned from convert_value in the case where the value is None. The argument against that is we'd be special-casing the Sum aggregate, while leaving other aggregates to use the Coalesce method. That's not such a big downside though.

Does anyone else want to weigh in on this? Supporting a default=0 kwarg is a 4 line + docs change.

[JuneHyeon Bae]

Replying to jarshwah:

@mrmachine that's not a bad argument to make. What we could support is:

Sum('field', default=0)

Which would then be returned from convert_value in the case where the value is None. The argument against that is we'd be special-casing the Sum aggregate, while leaving other aggregates to use the Coalesce method. That's not such a big downside though.

Does anyone else want to weigh in on this? Supporting a default=0 kwarg is a 4 line + docs change.

+1 for this.

[Mathieu]

Replying to jarshwah:

@mrmachine that's not a bad argument to make. What we could support is:

Sum('field', default=0)

Which would then be returned from convert_value in the case where the value is None. The argument against that is we'd be special-casing the Sum aggregate, while leaving other aggregates to use the Coalesce method. That's not such a big downside though.

Does anyone else want to weigh in on this? Supporting a default=0 kwarg is a 4 line + docs change.

+1 from me too.

[Tyler Morgan]

Replying to jarshwah:

@mrmachine that's not a bad argument to make. What we could support is:

Sum('field', default=0)

Which would then be returned from convert_value in the case where the value is None. The argument against that is we'd be special-casing the Sum aggregate, while leaving other aggregates to use the Coalesce method. That's not such a big downside though.

Does anyone else want to weigh in on this? Supporting a default=0 kwarg is a 4 line + docs change.

I ran into a situation today where this feature would have been nice. +1 from me.

[Josh Smeaton]

If you are running into this use case, you can use Coalesce(Sum('field'), 0) in Django 1.8. At least until a decision is made on this ticket. If you want to bring this up on the ML for the other ORM people to have input, that'd be a good next step.

[Jiyda Mint Moussa]

Replying to Josh Smeaton:

@mrmachine that's not a bad argument to make. What we could support is:

Sum('field', default=0)

Which would then be returned from convert_value in the case where the value is None. The argument against that is we'd be special-casing the Sum aggregate, while leaving other aggregates to use the Coalesce method. That's not such a big downside though.

Does anyone else want to weigh in on this? Supporting a default=0 kwarg is a 4 line + docs change.

+1 for me .

[Simon Charette]

May I suggest the argument be named coalesce and be added to all aggregates (through the django.db.models.aggregate.Aggregate class)?

[Aymeric Augustin]

I'm also facing this issue. The workaround with Coalesce works, but...

from django.db.models import Sum
from django.db.models.functions import Coalesce

really?

[Simon Charette]

Note that the new aggregate functions in django.contrib.postgres ​silently handle this.

I still believe adding a coalesce sugar argument to aggregate functions would be more appropriate than a default one as it would allow passing expressions instead of simple Python value.

Sum('amount', coalesce=F('minimum_amount')

[Hans Aarne Liblik]

+1 for

default

kwarg

[Michael Yartsev]

Another +1

[Björn W]

Aaargh, cmon, 11 years? To add a perfectly sane default of 0 for Sum? I'm amazed this hasn't been fixed for so long :) What was wrong with supporting default=0 at least? (yes I had the same perfectly normal reaction of getting annoyed by seeing NULL in my API output instead of 0 for a Sum of Payments for a User).

[Mariusz Felisiak]

Björn, such comments doesn't help. Patch is welcome.

[Barton Ip]

​PR

[Nick Pope]

I have produced an implementation using Coalesce for all aggregates other than Count.

See the new ​PR.

[Mariusz Felisiak]

Some tests don't work on MySQL and Oracle.

[Nick Pope]

Replying to Mariusz Felisiak:

Some tests don't work on MySQL and Oracle.

The tests are now all passing. \o/

[Mariusz Felisiak <felisiak.mariusz@…>]

In 501a8db4:

Fixed #10929 -- Added default argument to aggregates.

Thanks to Simon Charette and Adam Johnson for the reviews.

[Mariusz Felisiak <felisiak.mariusz@…>]

In fee87345:

Refs #10929 -- Deprecated forced empty result value for PostgreSQL aggregates.

This deprecates forcing a return value for ArrayAgg, JSONBAgg, and
StringAgg when there are no rows in the query. Now that we have a
default argument for aggregates, we want to revert to returning the
default of None which most aggregate functions return and leave it
up to the user to decide what they want to be returned by default.

[GitHub <noreply@…>]

In 022d29c9:

Refs #10929 -- Allowed NowUTC SQL customization for third-party backends.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 0db8bf3d:

Refs #10929 -- Fixed aggregates crash when passing strings as defaults.

Previously strings were interpreted as F() expressions and default
crashed with AttributeError:

'F' object has no attribute 'empty_result_set_value'

[Mariusz Felisiak <felisiak.mariusz@…>]

In 0be8095:

Refs #10929 -- Stopped forcing empty result value by PostgreSQL aggregates.

Per deprecation timeline.

[GitHub <noreply@…>]

In b05dfc28:

Refs #34381, Refs #10929 -- Fixed postgres_tests.test_aggregates.TestGeneralAggretate.test_empty_result_set() on PostgreSQL 14+.

Follow up to 0be8095b254fad65b2480d677ebe6098c41bbad6.