Hello Tom,
Thanks for working on adding filter support to aggregate. I think adding
support for the SQL:2003 features on all aggregates by emulating it on
backends that don't support it using CASE makes a lot of sense[0].
As I've mentioned on your PR this is syntactic sugar I've been missing from
Django's conditional aggregation API since I was using
django-aggregate-if[1]
before #11305 was fixed[2].
Now for the proposed syntax I understand you want to mimic the Queryset's
API by allowing filter()/exclude() to be chained but I don't think filter
chaining
on aggregate is common enough to warrant the extra effort required to make
it
work. As I've mentioned previously I'd advocate for a simple `filter` kwarg
that
accepts a Q instance instead as it makes it easier to implement and emulate
the Case() fallback. This also makes the filter() vs filter().filter() for
multi-valued
relationships subtleties a non-issue.
Mailbox.objects.annotate(
read_emails=Count('emails', filter=Q(unread=False)),
unread_emails=Count('emails', filter=Q(unread=True)),
recent_emails=Count('emails', filter=Q(received_date__lt=one_week_ago)),
)
Cheers,
Simon
[0] http://modern-sql.com/feature/filter
[1] https://pypi.python.org/pypi/django-aggregate-if
[2] https://code.djangoproject.com/ticket/11305
Le mercredi 12 avril 2017 17:31:44 UTC-4, Tom Forbes a écrit :
>
> Hello,
> At the Djangocon sprints I wanted to add support for a postgres specific
> syntax for filtering aggregations, which is quite simple: MAX(something)
> FILTER (WHERE x=1).
>
> During this the sprints I was told that it would be good to support this
> for all databases, and it can be done using the CASE syntax: MAX(CASE WHEN
> x=1 THEN something END).
>
> Doing this with the ORM is quite verbose, it would be really nice to be
> able to have a shorthand syntax for filtering aggregates that can use
> database-specific syntax where available (the postgres syntax is faster
> than the CASE syntax). I've made a proof of concept merge request that
> implements this here:
>
> https://github.com/django/django/pull/8352
>
> I'd really like some feedback and to maybe have a discussion about what
> the API could look like. Currently I went for adding `.filter()` and
> `.exclude()` methods to the base Aggregate class. I like this approach as
> it's close to the familiar queryset syntax but I understand there are
> subtleties with combining them (I just AND them together currently). It's
> also better to be consistent - if we can't support all of the queryset
> filter() syntax then we shouldn't confuse people by having a filter method
> that acts differently.
>
> An example of the API is this:
>
> Mailboxes.objects.annotate(
> read_emails=Count('emails').filter(unread=False),
> unread_emails=Count('emails').filter(unread=True),
> recent_emails=Count('emails').filter(received_date__lt=one_week_ago)
> )
>
>
> I'd really like some feedback on what the API should look like. Is filter
> a good idea? Any feedback on the current merge request implementation is
> appreciated as well, I'm fairly new to the Django expression internals. I
> think I'm going the right way with having a separate FilteredAggregate
> expression but I'm not sure.
>
> Many thanks,
> Tom
>
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/6d65367f-5e40-46a1-92c6-52e351002af2%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
