Re: [Django] #14357: Prevent inappropriate order-based grouping on values+annotate queries
Django
-------------------------------------+-------------------------------------
Reporter: Martin Chase | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version:
(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 Tim Graham):
* needs_better_patch: 1 => 0
Comment:
[https://github.com/django/django/pull/10005 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/14357#comment:17>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Martin Chase | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version:
(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 Tim Graham <timograham@…>):
In [changeset:"1b1f64ee5a78cc217fead52cbae23114502cf564" 1b1f64ee]:
{{{
#!CommitTicketReference repository=""
revision="1b1f64ee5a78cc217fead52cbae23114502cf564"
Refs #14357 -- Deprecated Meta.ordering affecting GROUP BY queries.
Thanks Ramiro Morales for contributing to the patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/14357#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Chase | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 1 => 0
Comment:
I'm unchecking ''Has patch'', since the PR (deprecating `ordering` being
used for grouped queries was merged). What remains to close this ticket?
--
Ticket URL: <https://code.djangoproject.com/ticket/14357#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Chase | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* keywords: => 3.1
* stage: Accepted => Someday/Maybe
Comment:
The behavior change will happen in Django 3.1 when the deprecation ends.
--
Ticket URL: <https://code.djangoproject.com/ticket/14357#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Chase | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution:
Keywords: 3.1 | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"0ddb4ebf7bfcc4730c80a772dd146a49ef6895f6" 0ddb4ebf]:
{{{
#!CommitTicketReference repository=""
revision="0ddb4ebf7bfcc4730c80a772dd146a49ef6895f6"
Refs #14357 -- Made Meta.ordering not affect GROUP BY queries.
Per deprecation timeline.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/14357#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Chase | Owner: (none)
Component: Database layer | Version:
(models, ORM) |
Keywords: 3.1 | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/14357#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Chase | Owner: (none)
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
I beleave this was not properly fixed in commit 0ddb4ebf.
While commit 0ddb4ebf removes `ORDER BY` when Meta.ordering is used it
still does populates `GROUP BY` with Meta.ordering fields thus leads to
wrong aggregation.
Look to get_group_by() at compiler.py [1] - it populates `GROUP BY` using
previously obtained order_by with `Meta.ordering` fields [2][3].
For what I see we must filter off `Meta.ordering` fields from order_by in
get_group_by() function.
[1]
https://github.com/django/django/blob/3.1.7/django/db/models/sql/compiler.py#L128
[2]
https://github.com/django/django/blob/3.1.7/django/db/models/sql/compiler.py#L56
[3]
https://github.com/django/django/blob/3.1.7/django/db/models/sql/compiler.py#L286
--
Ticket URL: <https://code.djangoproject.com/ticket/14357#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Chase | Owner: (none)
Component: Database layer | Version:
(models, ORM) |
Keywords: 3.1 | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
Please don't reopen closed tickets. You can create a new ticket if you can
provide a sample project or test and prove that `Meta.ordering` is still
included.
--
Ticket URL: <https://code.djangoproject.com/ticket/14357#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Chase | Owner: (none)
Type: Bug | Status: closed
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: 3.1 | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"fcd44b889f36c4be87910745614a0a4c88d7a3d8" fcd44b88]:
{{{
#!CommitTicketReference repository=""
revision="fcd44b889f36c4be87910745614a0a4c88d7a3d8"
Refs #14357 -- Updated docs about interaction between aggregations and
QuerySet.order_by().
Obsolete since 0ddb4ebf7bfcc4730c80a772dd146a49ef6895f6.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/14357#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Chase | Owner: (none)
Type: Bug | Status: closed
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: 3.1 | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"9b096063c13085d06236a72dcc46915f4c4ec28a" 9b096063]:
{{{
#!CommitTicketReference repository=""
revision="9b096063c13085d06236a72dcc46915f4c4ec28a"
[3.2.x] Refs #14357 -- Updated docs about interaction between aggregations
and QuerySet.order_by().
Obsolete since 0ddb4ebf7bfcc4730c80a772dd146a49ef6895f6.
Backport of fcd44b889f36c4be87910745614a0a4c88d7a3d8 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/14357#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Chase | Owner: (none)
Type: Bug | Status: closed
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: 3.1 | Triage Stage:
| Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Adam Sołtysik):
For anyone surprised, just like me, that `Meta.ordering` still breaks
aggregation queries in Django 3.1 and 3.2, despite what the documentation
says [https://docs.djangoproject.com/en/4.0/releases/3.1/#features-
removed-in-3-1 here] and
[https://docs.djangoproject.com/en/4.0/releases/2.2/#model-meta-ordering-
will-no-longer-affect-group-by-queries here], it seems that this was
finally fixed in #32546 in Django 4.0. Possibly the documentation should
be updated to reflect the actual version when the change was implemented,
as there seems to be nothing about it in the
[https://docs.djangoproject.com/en/4.0/releases/4.0/ release notes for
4.0]. Or maybe the change should be backported to reflect the
documentation.
--
Ticket URL: <https://code.djangoproject.com/ticket/14357#comment:27>