Opened 8 years ago

Closed 7 years ago

Last modified 2 years ago

#27849 closed New feature (fixed)

Add SQL 2003 FILTER syntax support with Case(When()) fallback to aggregates

Reported by: Tom Forbes Owned by: Tom Forbes
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In some circumstances being able to filter results included in an ArrayAgg is needed. PostgreSQL supports this through the FILTER WHERE clause:

SELECT ARRAY_AGG(id) FILTER (WHERE id < 10) as foo FROM table

Adding support for Q expressions in the ArrayAgg class could provide this functionality, i.e:

SomeModel.objects.annotate(foo=ArrayAgg('some_relation__id', where=Q(some_relation__bar=10)))

Change History (13)

comment:1 by Tom Forbes, 8 years ago

I have added a completely basic, first-attempt at this here: https://github.com/django/django/pull/8073

This currently works as described in the ticket above, but the code is not optimal and is copied from our internal implementation. As this feature is needed for the application I am working on I can continue to develop this, but I have some questions:

Should this be an extension to ArrayAgg or a separate aggregate? If it should be an extension, how can I change the template to add the FILTER (WHERE) clause if required?

There doesn't seem to be an easy way to subclass ArrayAgg and add elements to data or params, which is needed in this case. How could this be achieved without duplicating the entire as_sql code as it is currently? (it seems like the as_sql method should be broken up a bit, I think?)

It would be quite nice to be able to pass a whole, full-fat QuerySet into the aggregate, with the predicate that it is has been values_list'ed and is a relation of the model being queried (i.e SomeModel.objects.annotate(foo=ArrayAgg(SomeRelation.objects.filter(parent=F('id')).filter(something=something_else).values_list('xyz')) or somesuch. Is this even possible, or would it be chewing off too much?

Oh, and is this a feature that Django even wants?

Last edited 8 years ago by Tom Forbes (previous) (diff)

comment:2 by Mads Jensen, 8 years ago

For what it's worth, I posted a snippet that solves a more generic version of this a

https://djangosnippets.org/snippets/10603/ The syntax is not supported on Oracle, MySQL nor SQLite, so I suppose new feature-flags could be introduced etc.

Version 0, edited 8 years ago by Mads Jensen (next)

comment:3 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted

You can ask on the DevelopersMailingList to get feedback about the design decisions such as the syntax.

in reply to:  description comment:4 by Tom Forbes, 8 years ago

I've added a new merge request: https://github.com/django/django/pull/8306. I'm at the Djangocon sprints for the next couple of days if anyone wishes to talk to me in person about this, or has any comments.

comment:5 by Simon Charette, 8 years ago

Component: contrib.postgresDatabase layer (models, ORM)
Has patch: set
Owner: set to nobody
Patch needs improvement: set
Summary: Support Postgres FILTER WHERE conditions in ArrayAggAdd SQL 2003 FILTER syntax support with Case(When()) fallback to aggregates

comment:6 by Simon Charette, 8 years ago

Patch needs improvement: unset

comment:7 by Mariusz Felisiak, 7 years ago

Patch needs improvement: set

comment:8 by Tom Forbes, 7 years ago

Owner: changed from nobody to Tom Forbes
Patch needs improvement: unset
Status: newassigned

I've made the changes requested in the github ticket, any reviews would be appreciated.

comment:9 by Mariusz Felisiak, 7 years ago

Triage Stage: AcceptedReady for checkin

comment:10 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In b78d100f:

Fixed #27849 -- Added filtering support to aggregates.

comment:11 by Tim Graham <timograham@…>, 7 years ago

In b43acf22:

Refs #27849 -- Removed empty Q() hack in filtered Aggregate.as_sql().

This required allowing WhereNode to be provided as When(condition).

This was made possible by cf12257db23fa248c89a3da3f718aa01a50ca659.

comment:12 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 77cf70ea:

Refs #27849 -- Added test for filtered aggregates with empty conditions.

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In 967f875:

Refs #27849 -- Fixed filtered aggregates crash on filters that match everything.

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