Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GroupConcat missing filter parameter #947

Open
caramdache opened this issue Sep 13, 2022 · 4 comments · May be fixed by #948
Open

GroupConcat missing filter parameter #947

caramdache opened this issue Sep 13, 2022 · 4 comments · May be fixed by #948

Comments

@caramdache
Copy link

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 caramdache changed the title group_concat missing filter parameter GroupConcat missing filter parameter Sep 13, 2022
@adamchainz
Copy link
Owner

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.

@caramdache
Copy link
Author

caramdache commented Sep 14, 2022

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.

@adamchainz adamchainz linked a pull request Sep 14, 2022 that will close this issue
@adamchainz
Copy link
Owner

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.

@caramdache
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants