[Django] #24887: Remove one-arg limitation from django.db.models.aggregate

Django

unread,

Jun 1, 2015, 3:33:52 AM6/1/15

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: master
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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.

--
Ticket URL: <https://code.djangoproject.com/ticket/24887>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,

Jun 1, 2015, 10:45:16 AM6/1/15

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner: nobody
Type: | Status: new
Cleanup/optimization |

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by charettes):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/24887#comment:1>

Django

unread,

Jun 4, 2015, 6:33:17 AM6/4/15

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------

Reporter: akaariai | Owner: gchp
Type: | Status: assigned

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by gchp):

* status: new => assigned
* owner: nobody => gchp

--
Ticket URL: <https://code.djangoproject.com/ticket/24887#comment:2>

Django

unread,

Jun 4, 2015, 11:16:50 AM6/4/15

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by akaariai):

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.

--
Ticket URL: <https://code.djangoproject.com/ticket/24887#comment:3>

Django

unread,

Jun 4, 2015, 11:27:43 AM6/4/15

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by gchp):

* has_patch: 0 => 1

Comment:

PR here: https://github.com/django/django/pull/4767

--
Ticket URL: <https://code.djangoproject.com/ticket/24887#comment:4>

Django

unread,

Jun 4, 2015, 11:39:19 AM6/4/15

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by coldmind):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/24887#comment:5>

Django

unread,

Jun 4, 2015, 1:02:24 PM6/4/15

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by charettes):

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`.

--
Ticket URL: <https://code.djangoproject.com/ticket/24887#comment:6>

Django

unread,

Jun 4, 2015, 7:23:31 PM6/4/15

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by jarshwah):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/24887#comment:7>

Django

unread,

Jun 5, 2015, 6:50:23 AM6/5/15

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by gchp):

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?

--
Ticket URL: <https://code.djangoproject.com/ticket/24887#comment:8>

Django

unread,

Jun 6, 2015, 2:13:34 PM6/6/15

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by timgraham):

* needs_tests: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/24887#comment:9>

Django

unread,

Jun 8, 2015, 5:27:59 AM6/8/15

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by gchp):

* needs_tests: 1 => 0

Comment:

Just updated the PR with a test

--
Ticket URL: <https://code.djangoproject.com/ticket/24887#comment:10>

Django

unread,

Jun 19, 2015, 9:20:16 AM6/19/15

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by timgraham):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/24887#comment:11>

Django

unread,

Jun 21, 2015, 8:16:02 PM6/21/15

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by jarshwah):

* needs_better_patch: 1 => 0

Comment:

I've extended @ghcp's PR with code that handles default_alias and a
recursive contains_aggregate. https://github.com/django/django/pull/4900

--
Ticket URL: <https://code.djangoproject.com/ticket/24887#comment:12>

Django

unread,

Jun 27, 2015, 11:45:02 AM6/27/15

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------

Reporter: akaariai | Owner: gchp
Type: | Status: closed

Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |

Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Tim Graham <timograham@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"4a66a69239c493c05b322815b18c605cd4c96e7c" 4a66a692]:
{{{
#!CommitTicketReference repository=""
revision="4a66a69239c493c05b322815b18c605cd4c96e7c"
Fixed #24887 -- Removed one-arg limit from models.aggregate
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/24887#comment:13>

Django

unread,

Jul 21, 2017, 7:53:45 AM7/21/17

to django-...@googlegroups.com

#24887: Remove one-arg limitation from django.db.models.aggregate
-------------------------------------+-------------------------------------

Reporter: Anssi Kääriäinen | Owner: Greg
Type: | Chapple
Cleanup/optimization | Status: closed

Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette <charette.s@…>):

In [changeset:"160969d97080b53e9113cadcecc27c26a51881e8" 160969d]:
{{{
#!CommitTicketReference repository=""
revision="160969d97080b53e9113cadcecc27c26a51881e8"
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().
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/24887#comment:14>

Reply all

Reply to author

Forward