#24887 closed Cleanup/optimization (fixed)
Remove one-arg limitation from django.db.models.aggregate
Reported by: | Anssi Kääriäinen | Owned by: | Greg Chapple |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The resolve_expression() method of django.db.models.aggregates.Aggregate asserts that there is just single source expression. I don't see the reason for this limitation. There are aggregates (like SQLite's group_concat) that naturally take in multiple arguments. It seems group_concat works just fine with multiple source expressions if the assertion is removed.
Change History (14)
comment:1 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 9 years ago
comment:5 by , 9 years ago
I think it because https://github.com/django/django/blob/3e9b5bfd9c3e16c968278f7d050f52739690eb29/django/db/models/aggregates.py#L21 is using only first expression, so it will be confusing to allow passing more
comment:6 by , 9 years ago
Does len(self.source_expressions) == (c.source_expressions)
in all cases? If that's the case maybe we should loop over all c.source_expressions
to test for .contains_aggregate and not summarize
.
comment:7 by , 9 years ago
Yes, a loop will do. We'll also need to fail if an aggregate is provided multiple source_expressions but no alias. See https://github.com/django/django/blob/3e9b5bfd9c3e16c968278f7d050f52739690eb29/django/db/models/aggregates.py#L22 and https://github.com/django/django/blob/3e9b5bfd9c3e16c968278f7d050f52739690eb29/django/db/models/aggregates.py#L34
comment:8 by , 9 years ago
I've updated the PR to loop over source_expressions. Wondering how I can test the new behavour - can anyone give some pointers on how I might make source_expressions have more than one element?
comment:9 by , 9 years ago
Needs tests: | set |
---|
comment:11 by , 9 years ago
Patch needs improvement: | set |
---|
comment:12 by , 9 years ago
Patch needs improvement: | unset |
---|
I've extended @ghcp's PR with code that handles default_alias and a recursive contains_aggregate. https://github.com/django/django/pull/4900
This should be one-line change to django/db/models/aggregates, remove the line that does the assert for len(self.source_expressions) and things should work.