Opened 4 years ago

Closed 4 years ago

Last modified 19 months ago

#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: master
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


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 Changed 4 years ago by Simon Charette

Triage Stage: UnreviewedAccepted

comment:2 Changed 4 years ago by Greg Chapple

Owner: changed from nobody to Greg Chapple
Status: newassigned

comment:3 Changed 4 years ago by Anssi Kääriäinen

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.

comment:4 Changed 4 years ago by Greg Chapple

Has patch: set

comment:5 Changed 4 years ago by Andriy Sokolovskiy

I think it because is using only first expression, so it will be confusing to allow passing more

comment:6 Changed 4 years ago by Simon Charette

Does len(self.source_expressions) == len(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.

Last edited 4 years ago by Simon Charette (previous) (diff)

comment:8 Changed 4 years ago by Greg Chapple

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 Changed 4 years ago by Tim Graham

Needs tests: set

comment:10 Changed 4 years ago by Greg Chapple

Needs tests: unset

Just updated the PR with a test

comment:11 Changed 4 years ago by Tim Graham

Patch needs improvement: set

comment:12 Changed 4 years ago by Josh Smeaton

Patch needs improvement: unset

I've extended @ghcp's PR with code that handles default_alias and a recursive contains_aggregate.

comment:13 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 4a66a692:

Fixed #24887 -- Removed one-arg limit from models.aggregate

comment:14 Changed 19 months ago by Simon Charette <charette.s@…>

In 160969d:

Refs #24887 -- Stopped mutating a test expression during as_sql().

Also defined an explicit output_field as it was mixing an expression with an
IntegerField() with one with a DecimalField().

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