Re: [Django] #31880: QuerySet.aggregate() mixes annotated fields names.

[Django]

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

 * 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.2de7c5325985822ba59870f4a44841d3%40djangoproject.com.

Re: [Django] #31880: QuerySet.aggregate() mixes annotated fields names.

[Django]

#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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.026ffce44a80d9b50baf3856e2b08820%40djangoproject.com.

Re: [Django] #31880: QuerySet.aggregate() mixes annotated fields names.

[Django]

#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
-+-

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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.4deb8f26843c64c7a0ad194eac0a124a%40djangoproject.com.

Re: [Django] #31880: QuerySet.aggregate() mixes annotated fields names.

[Django]

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

 * 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.836a555374bf4d4c8569876cfe924bfa%40djangoproject.com.

Re: [Django] #31880: QuerySet.aggregate() mixes annotated fields names.

[Django]

#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:  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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.453c16a57621bb31940e77e8ea71d6dd%40djangoproject.com.

Re: [Django] #31880: QuerySet.aggregate() mixes annotated fields names.

[Django]

#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 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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.feed9bc1ecc5efbb9b68d28d2f53ea43%40djangoproject.com.

Re: [Django] #31880: QuerySet.aggregate() mixes annotated fields names.

[Django]

#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
-+-

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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.e052fc689e60c0f6db5d0b42dbdfafc4%40djangoproject.com.

Re: [Django] #31880: QuerySet.aggregate() mixes annotated fields names.

[Django]

#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
-+-

Comment (by felixxm):

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

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.8d61dfa07f26a3de56d8740077538319%40djangoproject.com.

Re: [Django] #31880: QuerySet.aggregate() mixes annotated fields names.

[Django]

#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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.53e3c81e19cd1959e61abcbeba1c9198%40djangoproject.com.

Re: [Django] #31880: QuerySet.aggregate() mixes annotated fields names. (was: Incorrect handling of aggregate's keyword name)

[Django]

#31880: QuerySet.aggregate() mixes annotated fields names.
-+-
 Reporter:  Thodoris |Owner:  nobody
  Sotiropoulos   |
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  3.1
  (models, ORM)  |
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Accepted
Has patch:  0|  Needs documentation:  0
  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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/066.9009383bbde305c149c48dd5c12ecafa%40djangoproject.com.