Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #31880 -- Made QuerySet.aggregate() raise FieldError when aggregating over aggregation aliases. #13431

Merged
merged 1 commit into from Sep 29, 2020

Conversation

David-Wobrock
Copy link
Member

@David-Wobrock David-Wobrock commented Sep 17, 2020

Ticket: https://code.djangoproject.com/ticket/31880

(First implementation: #13390)

Suggested patch for the discussion https://code.djangoproject.com/ticket/31880#comment:7

tests/aggregation/tests.py Outdated Show resolved Hide resolved
django/db/models/query.py Outdated Show resolved Hide resolved
@felixxm
Copy link
Member

felixxm commented Sep 17, 2020

@jacobtylerwalls proposed the same solution to fix ticket-31679, see #13273. My initial response was that it's too restrictive. I was thinking about checking that an aggregation overrides and uses the same aliases 🤔. If it's not feasible or it's too complicated then I'm fine with this patch but we should add a backward incompatible note.

@David-Wobrock
Copy link
Member Author

Hi @felixxm
The patch is indeed exactly the same :P And I agree that it is too restrictive.
I tried to focus back on the issue at hand:
Considering Author.objects.annotate(age_alias=F('age')).aggregate(age=Sum(F('age_alias')), avg_age=Avg(F('age'))), the last aggregation is referring to the aggregation made just before, in the same aggregate call.

This should be less restrictive and still allow currently working QuerySets like Author.objects.annotate(age_alias=F('age')).aggregate(age_alias=Sum(F('age_alias')).
But still prevent the initial problem presented in the ticket https://code.djangoproject.com/ticket/31880

What do you think about this approach? 🤔

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@David-Wobrock Looks good, thanks 👍

django/db/models/query.py Outdated Show resolved Hide resolved
django/db/models/query.py Outdated Show resolved Hide resolved
@David-Wobrock
Copy link
Member Author

@felixxm Merged the suggestions into one commit, thanks!

@felixxm felixxm changed the title Fixed #31880 -- Raised error when aggregation alias overrides existing annotation Fixed #31880 -- Made QuerySet.aggregate() raise FieldError when aggregating over aggregation aliases. Sep 29, 2020
@felixxm
Copy link
Member

felixxm commented Sep 29, 2020

@David-Wobrock Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants