Opened 3 years ago

Closed 3 years ago

Last modified 14 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

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

Triage Stage: UnreviewedAccepted

comment:2 Changed 3 years ago by Greg Chapple

Owner: changed from nobody to Greg Chapple
Status: newassigned

comment:3 Changed 3 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 3 years ago by Greg Chapple

Has patch: set

comment:5 Changed 3 years ago by Andriy Sokolovskiy

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

comment:8 Changed 3 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 3 years ago by Tim Graham

Needs tests: set

comment:10 Changed 3 years ago by Greg Chapple

Needs tests: unset

Just updated the PR with a test

comment:11 Changed 3 years ago by Tim Graham

Patch needs improvement: set

comment:12 Changed 3 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. https://github.com/django/django/pull/4900

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

Resolution: fixed
Status: assignedclosed

In 4a66a692:

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

comment:14 Changed 14 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