django-mysql icon indicating copy to clipboard operation
django-mysql copied to clipboard

GroupConcat missing `filter` parameter

Open caramdache opened this issue 2 years ago • 4 comments

Description

Native Django aggregates support a filter parameter that allow a subset of expressions to be returned. For example:

.annotate(count=Count("expr", filter=filter_expr))

which generates the following SQL:

COUNT(DISTINCT CASE WHEN <filter_expr> THEN <expr> ELSE NULL END) as count

However the filter parameter is ignored today by GroupConcat and I'm not quite sure how to add it to the source code.

caramdache avatar Sep 13 '22 17:09 caramdache

Can you check that GROUP_CONCAT supports this syntax, by showing some example queries?

I might get around to adding this, but if you can try to open a PR that's a much better way of adding this feature. I don't use django-mysql very often these days, as most clients I work with use PostgreSQL.

adamchainz avatar Sep 13 '22 17:09 adamchainz

I just created a PR with a fix, based on Django Count.

Instead of creating the SQL manually, the Django Aggregate compiler is used instead. But that leaves out support for ORDER BY and SEPARATOR. Probably, this can be added by updating the template expression. But I did not do this for lack of time, understanding and filter was more important for me than the other two.

Should someone figure how to do this, then the manual query creation code could probably go entirely. At the moment, it continues to be used when there is no filter expression.

caramdache avatar Sep 14 '22 09:09 caramdache

Instead of creating the SQL manually, the Django Aggregate compiler is used instead. But that leaves out support for ORDER BY and SEPARATOR. Probably, this can be added by updating the template expression. But I did not do this for lack of time, understanding and filter was more important for me than the other two.

I've commented on the PR that I was confused by this. Now I see your comment that this was due to lack of time, I won't merge this in its current state. I'd like the filter pathway to be part of the existing compilation code.

adamchainz avatar Sep 14 '22 10:09 adamchainz

I've added support for ORDER BY and SEPARATOR earlier today.

caramdache avatar Sep 14 '22 16:09 caramdache