[Django] #31880: Incorrect handling of aggregate's keyword name

Django

unread,

Aug 13, 2020, 8:25:19 AM8/13/20

to django-...@googlegroups.com

#31880: Incorrect handling of aggregate's keyword name
-------------------------------------+-------------------------------------
Reporter: Thodoris | Owner: nobody
Sotiropoulos |
Type: | Status: new
Uncategorized |
Component: Database | Version: 3.1
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 |
-------------------------------------+-------------------------------------
I have the following model (simplified for opening the issue).

{{{#!python
class T(models.Model):
id = models.AutoField(primary=True)
foo = models.IntegerField()
}}}

and I perform the following query on MySQL (simplified for opening the
issue)

{{{#!python
T.objects.annotate(anon=F('foo')).aggregate(foo=Max(F('anon')),
sum=Sum(F('foo')))
}}}

this produces the following SQL query

{{{#!sql
SELECT MAX(`anon`), SUM(`foo`) FROM (SELECT `table`.`foo` AS `anon` FROM
`foo`) subquery
}}}

that causes the following exception

{{{
django.db.utils.OperationalError: (1054, "Unknown column 'foo' in 'field
list'")
}}}

I would expect django to generate

{{{#!sql
SELECT MAX(`anon`), SUM(`anon`) FROM (SELECT `table`.`foo` AS `anon` FROM
`table`) subquery
}}}

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

Django

unread,

Aug 14, 2020, 1:19:55 AM8/14/20

to django-...@googlegroups.com

#31880: QuerySet.aggregate() mixes annotated fields names.

-------------------------------------+-------------------------------------
Reporter: Thodoris | Owner: nobody
Sotiropoulos |

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

Changes (by felixxm):

* type: Uncategorized => Bug
* stage: Unreviewed => Accepted

Comment:

> I would expect django to generate
>
> {{{#!sql
> SELECT MAX(`anon`), SUM(`anon`) FROM (SELECT `table`.`foo` AS `anon`
FROM `table`) subquery
> }}}

I would rather expect:

{{{#!sql
SELECT MAX(`anon`), SUM(`foo`) FROM (SELECT `table`.`foo`, `table`.`foo`

AS `anon` FROM `table`) subquery
}}}

Nevertheless we should raise a descriptive error or add a column to the
`subquery`. Is there any reason to not use `Sum(F('anon'))`?

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

Django

unread,

Sep 5, 2020, 2:35:24 PM9/5/20

to django-...@googlegroups.com

#31880: QuerySet.aggregate() mixes annotated fields names.
-------------------------------------+-------------------------------------

Reporter: Thodoris | Owner: David
Sotiropoulos | Wobrock
Type: Bug | Status: assigned

Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:
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 David Wobrock):

* cc: David Wobrock (added)
* owner: nobody => David Wobrock
* has_patch: 0 => 1
* status: new => assigned

Comment:

Hi,

I tried to tackle this ticket by fixing the actual bug.

The [https://github.com/django/django/pull/13390 PR]

It's surely not that an important bug and one could just use a different
alias for the aggregation, but I wanted to get to the bottom.

Aggregations are actually also registering annotations, meaning that the
annotation alias is overridden, and the subquery is not aware of the field
anymore.
I thought of three ways to solve this:
* raise an error when an aggregation is overriding the alias of an
annotation (basically, forbid the buggy case) -- easy to implement, but I
think it's a pity to forbid something that could work.
* implement the smallest change that will allow this case -- it's the
approach implemented in the linked patch.
* handle differently aggregation annotations and simple annotations -- a
large change, which would require quite a rewrite of parts of the
aggregation logic and would be ''a lot'' of work for such a small edge
case I guess.

I hope it makes sense to you and I'm open to changing to any other and
better approach :)
David

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

Django

unread,

Sep 6, 2020, 3:11:00 AM9/6/20

to django-...@googlegroups.com

Has patch: 1 | Needs documentation: 0

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

Comment (by felixxm):

Jacob, it's really similar but it's not a duplicate, IMO.

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

Django

unread,

Sep 6, 2020, 8:40:49 AM9/6/20

to django-...@googlegroups.com

Has patch: 1 | Needs documentation: 0

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

Comment (by Jacob Walls):

Yeah I can see that better now, sorry. I also checked out the patch and
see that it doesn't address #31679 as far as I can tell, so better to have
two tickets.

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

Django

unread,

Sep 15, 2020, 7:32:47 AM9/15/20

to django-...@googlegroups.com

Has patch: 1 | Needs documentation: 0

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

Changes (by felixxm):

* cc: Simon Charette (added)

Comment:

I would expect that the first kwarg (e.g. `age_alias=Sum(F('age')`)
overrides the previous annotation (e.g. `age_alias=F('age')`) and a
`FieldError` ''"Cannot compute Avg('age_alias'): 'age_alias' is an
aggregate"'' is raised, like with annotations:

-
`Author.objects.annotate(age_alias=F('age')).annotate(age_alias=Sum(F('age'))).annotate(avg_age=Avg(F('age_alias')))`,
-
`Author.objects.annotate(age_alias=F('age')).annotate(age_alias=Sum(F('age')),
avg_age=Avg(F('age_alias')))`.

IMO it's not obvious how such querysets should behave. I think we should
raise an error if an `aggregate()` overrides and refs to the same alias.

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

Django

unread,

Sep 17, 2020, 3:10:55 AM9/17/20

to django-...@googlegroups.com

Has patch: 0 | Needs documentation: 0

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

Changes (by felixxm):

* has_patch: 1 => 0

Comment:

To sum up, I think supporting this edge case isn't worth adding extra
complexity, we should raise an error. You can always use different
aliases.

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

Django

unread,

Sep 17, 2020, 11:49:09 AM9/17/20

to django-...@googlegroups.com

Has patch: 1 | Needs documentation: 0

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

Changes (by David Wobrock):

* has_patch: 0 => 1

Comment:

Hi, thanks for taking the time diving into the issue.

I agree that the expected behaviour is not obvious.
My reflection why it could be supported was that:
* it is possible since the aliases are used in two different queries (the
outer and the inner) in the case of an aggregation
* ''You can always use different aliases.'' it's true, but with a system
that automatically generates QuerySets it might not be easy to "just use
different aliases".

However, let's go with an error, I agree that it adds complexity for not a
lot of added value. There is a fair chance that the first patch missed
edge cases.
I pushed a new patch: https://github.com/django/django/pull/13431

Happy to hear what you think about this one :)

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

Django

unread,

Sep 17, 2020, 12:00:18 PM9/17/20

to django-...@googlegroups.com

Has patch: 1 | Needs documentation: 0

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

Comment (by Simon Charette):

I think raising an explicit error at the ORM level is the right call for
now. Nothing prevents us from eventually fixing this if we can align on an
expected behaviour in the future but users will get a clearer error
message in the mean time.

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

Django

unread,

Sep 29, 2020, 3:07:57 AM9/29/20

to django-...@googlegroups.com

#31880: QuerySet.aggregate() mixes annotated fields names.
-------------------------------------+-------------------------------------
Reporter: Thodoris | Owner: David
Sotiropoulos | Wobrock
Type: Bug | Status: assigned
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0

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

Changes (by felixxm):

* stage: Accepted => Ready for checkin

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

Django

unread,

Sep 29, 2020, 3:57:20 AM9/29/20

to django-...@googlegroups.com

#31880: QuerySet.aggregate() mixes annotated fields names.
-------------------------------------+-------------------------------------
Reporter: Thodoris | Owner: David
Sotiropoulos | Wobrock

Type: Bug | Status: closed

Component: Database layer | Version: 3.1
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

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

Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

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

Comment:

In [changeset:"058747b77a3e6710301138b8ab3bc4256077fdfa" 058747b]:
{{{
#!CommitTicketReference repository=""
revision="058747b77a3e6710301138b8ab3bc4256077fdfa"
Fixed #31880 -- Made QuerySet.aggregate() raise FieldError when
aggregating over aggregation aliases.
}}}

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

Reply all

Reply to author

Forward