Opened 9 years ago

Closed 9 years ago

Last modified 7 years 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: 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 Simon Charette, 9 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Greg Chapple, 9 years ago

Owner: changed from nobody to Greg Chapple
Status: newassigned

comment:3 by Anssi Kääriäinen, 9 years ago

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 by Greg Chapple, 9 years ago

Has patch: set

comment:5 by Andriy Sokolovskiy, 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 Simon Charette, 9 years ago

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 9 years ago by Simon Charette (previous) (diff)

comment:8 by Greg Chapple, 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 Tim Graham, 9 years ago

Needs tests: set

comment:10 by Greg Chapple, 9 years ago

Needs tests: unset

Just updated the PR with a test

comment:11 by Tim Graham, 9 years ago

Patch needs improvement: set

comment:12 by Josh Smeaton, 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

comment:13 by Tim Graham <timograham@…>, 9 years ago

Resolution: fixed
Status: assignedclosed

In 4a66a692:

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

comment:14 by Simon Charette <charette.s@…>, 7 years ago

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