Add support for filter expression in GroupConcat#948
Conversation
GroupConcat did not support fitler expression. This PR adds support based on Django `Count` aggregate. https://github.com/adamchainz/django-mysql/blob/main/src/django_mysql/models/aggregates.py
for more information, see https://pre-commit.ci
adamchainz
left a comment
adamchainz
left a comment
There was a problem hiding this comment.
Nice work starting on this.
The tests are failing because the package requires test coverage - please add tests in the existing tests for group concat. It looks like we'll need at least two, depending on the value of distinct.
Also please update the documentation the changelog.
| if self.filter: | ||
| extra_context["distinct"] = "DISTINCT " if self.distinct else "" | ||
| copy = self.copy() | ||
| copy.filter = None | ||
| source_expressions = copy.get_source_expressions() | ||
| condition = When(self.filter, then=source_expressions[0]) | ||
| copy.set_source_expressions([Case(condition)] + source_expressions[1:]) | ||
| return super(Aggregate, copy).as_sql(compiler, connection, **extra_context) |
There was a problem hiding this comment.
I haven't looked into it, but it's a bit odd that you're skipping the existing implementation and calling super(). The implemetnation below covers the SQL oddities in GROUP_CONCAT, I think we'd still want to use it, just tweak adding the filter there.
There was a problem hiding this comment.
I'm not sure we need keep the existing pathway in the long term. It's probably better to let Aggregate.as_sql() perform the work based on the template.
If you look at the code for Count, you'll see there are 2 paths, one when filter is set, the other when it's not. I think we should have a similar structure here, but avoid manual handcrafting as much as possible because this is likely fragile.
Co-authored-by: Adam Johnson <me@adamj.eu>
for more information, see https://pre-commit.ci
I missed this in the first commit
for more information, see https://pre-commit.ci
I've updated the history file. I'm not sure if this is the file you had in mind. |
|
THat's the file, but you've done so in a separate PR: #949. Please do so in this PR, on the same branch. |
due to the introduction of an auxiliary function.
for more information, see https://pre-commit.ci
|
Still waiting on docs, changelog note, and tests. |
|
@caramdache Any status updates? Wondering if I can help implement this somehow without having to start a whole new MR. |
|
I think it's unlikely they'll return, please start a new PR. You can pull their changes with |
3 participants
Fixes #947.
GroupConcat did not support fitler expression. This PR adds support based on Django
Countaggregate.https://github.com/adamchainz/django-mysql/blob/main/src/django_mysql/models/aggregates.py