[Django] #35444: Add generic support for order by to Aggregate

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New | Status: assigned
feature |
Component: Database | Version: dev
layer (models, ORM) | Keywords: orderableaggmixin
Severity: Normal | aggregate
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
We would like to add general support for ordering within SQL aggregate
functions that allow it. Currently, this behavior is only supported in
PostgreSQL via the `OrderableAggMixin`. With changes made in #35339 , it
should be possible to merge the behaviors of that mixin into the core
behaviors of the `Aggregate` class and then apply it to functions that
support order by like Sqlite's `STRING_AGG`, MySQL's `GROUP_CONCAT`, and
Oracle's `LIST_AGG WITHIN GROUP (ORDER BY)`.

These are some implementation notes from Simon discussed on the original
bug ticket in this comment ticket:35339#comment:13 that I'm capturing some
of here to avoid jumping around.

> A few things I believe we should focus on along the way
> 1. We should make the argument Aggregate(order_by) instead of ordering
to be consistent with Window
> 2. Just like we do with allow_distinct we should use a allow_order_by
attribute for aggregates that support this feature
> 3. We should make OrderableAggMixin a shim that sets
allow_order_by=True, redirect __init__(ordering) to order_by, and emit a
deprecation warning when doing so to allow a proper transition for
contrib.postgres.ArrayAgg and friends
> 4. We should reuse OrderByList as much as possible
--
Ticket URL: <https://code.djangoproject.com/ticket/35444>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M

Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |
Has patch: 0 | Needs documentation: 0

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

Changes (by Simon Charette):

* cc: Simon Charette (added)
* stage: Unreviewed => Accepted

Comment:

As discussed on the other ticket I think that moving this capability to
the generic `Aggregate` class makes sense.
--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:1>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Hey Chris, have you run into any issues working on this ticket? Do you
need any support.
--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:2>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Chris M):

Thanks for the bump, Simon. I got into a busy period at work, but I was
just talking to my team about how I needed to get back to work on this PR
if I wanted to get it in for 5.2. So the timing works out well.

https://github.com/django/django/compare/main...camuthig:django:merge-
orderable-agg-mixin

I did hit a couple of bumps along the way but nothing blocking yet. I have
what appears to be a working feature tested locally against Sqlite and
Postgres and need to get the time to update to documentation and write
tests. In my code I am porting over the `StringAgg` function into the
standard DB module and I am shifting the postgres specific implementation
to be a subclass of it, so the existing postgres StringAgg tests work as a
full regression test for that database backend, but we don't have that
same coverage for the other backends yet.

I'm curious your thoughts on a few things:

1. Generally if you think the current path of the code is right or needs
changes. I can convert this diff link into a draft PR if that would help.
2. If you can point me in the right direction for any documentation you
think should be updated. I haven't started working through that just yet.
3. What do you think the best way to test this is? The Postgres aggregate
tests are very thorough, so I was thinking it might work to take all of
the string agg related tests from there and move them into a new test file
that isn't just run for the Postgres backend?
--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:3>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

> Thanks for the bump, Simon. I got into a busy period at work, but I was
just talking to my team about how I needed to get back to work on this PR
if I wanted to get it in for 5.2. So the timing works out well.

No problem Chris, thanks for the update! I reaching out because I'm also
want to find a way to make this land in 5.2.

> Generally if you think the current path of the code is right or needs
changes. I can convert this diff link into a draft PR if that would help.

From a cursory look at the branch it seems great, I would encourage you to
open a draft PR when you have the chance though (no rush) as it makes
collaboration through review much easier.

> If you can point me in the right direction for any documentation you
think should be updated. I haven't started working through that just yet.

Sure, since the patch includes a new feature and deprecation I can think
of a few places.

1. The 5.2 release notes.
2. The documentation for `ArrayAgg` and `JSONBAgg` to
[https://docs.djangoproject.com/en/5.0/ref/contrib/postgres/aggregates/#django.contrib.postgres.aggregates.ArrayAgg.ordering
document] the change of `ordering` to `order_by`
3. The complete deprecation of `postgres.StringAgg`, you could refer to
how the documentation change was made when we deprecated
`postgres.JSONField` in favor or `models.JSONField`
4.
[https://docs.djangoproject.com/en/4.2/ref/models/expressions/#django.db.models.Aggregate.allow_distinct
Something similar to the documentation] for `Aggregate.allow_distinct` for
`.allow_order_by`.
5. Documentation for `models.StringAgg`

> What do you think the best way to test this is? The Postgres aggregate
tests are very thorough, so I was thinking it might work to take all of
the string agg related tests from there and move them into a new test file
that isn't just run for the Postgres backend?

I think that's a good approach! Creating a PR out of your work should help
with ensuring the per-backend implementations are working fine.

Maybe one last point of feedback looking at the MR is to try to ''split''
your work in logical commits that can be merged serially and built on top
of each other. If you're not familiar with `git-rebase` I could help you
slice it at first. I think a good logical breakdown could be

1. Deprecate `OrderableAggMixin`'s subclasses usage of `ordering` in favor
of `order_by`
2. Deprecate `OrderableAggMixin` and move support to `Aggregate`
3. Deprecate `postgres.StringAgg` in favor of `models.aggregate.StringAgg`

Such a sequence should ease the review and allow merger to progressively
merge slices of the work into `main` and reduces the risks of having to
completely revert work.
--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:4>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Chris M):

Thanks for the feedback. I'll start working on some of that documentation.
I'll definitely do a full rebase before I finish up and get that testing
together with the last part of it.
--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:5>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |

Has patch: 1 | Needs documentation: 0

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

Changes (by Chris M):

* has_patch: 0 => 1

Comment:

[https://github.com/django/django/pull/18361 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:6>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

Patch appears to be in a good state and a solution was provided to
adequately deal with composite primary key counts.
--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:7>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

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

Changes (by Sarah Boyce):

* needs_better_patch: 0 => 1

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

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

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

Changes (by Jacob Walls):

* needs_better_patch: 1 => 0

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

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: orderableaggmixin | Triage Stage: Ready for
aggregate | checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

Comment:

Pulled the first commit to a separate PR which I aim to land:
https://github.com/django/django/pull/18996
--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:10>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Ready for
aggregate | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"d734f1651ccc0a74325f7b55f7eecc68edef6453" d734f16]:
{{{#!CommitTicketReference repository=""
revision="d734f1651ccc0a74325f7b55f7eecc68edef6453"
Refs #35444 -- Deprecated contrib.postgres aggregates ordering for
order_by.

Aligns the argument with SQL standards already used in Window.order_by and
sets up for adding support to Aggregate.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:11>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

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

Changes (by Sarah Boyce):

* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted

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

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* needs_better_patch: 1 => 0

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

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |
Has patch: 1 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* needs_tests: 0 => 1

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

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

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

Changes (by Simon Charette):

* needs_tests: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:15>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

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

Changes (by Sarah Boyce):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:16>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: orderableaggmixin | Triage Stage: Accepted
aggregate |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

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

Changes (by Simon Charette):

* needs_better_patch: 1 => 0

Comment:

Provided insights as to what needed to be adjusted for the patch to be
ready.
--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:17>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:

Keywords: orderableaggmixin | Triage Stage: Ready for
aggregate | checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:18>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M

Type: New feature | Status: closed

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

Severity: Normal | Resolution: fixed

Keywords: orderableaggmixin | Triage Stage: Ready for
aggregate | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"1759c1dbd1e3cd4c9fcd345269e0d25796468f7f" 1759c1db]:
{{{#!CommitTicketReference repository=""
revision="1759c1dbd1e3cd4c9fcd345269e0d25796468f7f"
Refs #35444 -- Deprecated contrib.postgres.OrderableAggMixin.

This commit does not create any functional changes, but marks the
existing `OrderableAggMixin` class as deprecated so that developers
using it directly can be made aware of its future removal.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:20>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: orderableaggmixin | Triage Stage: Ready for
aggregate | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

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

Comment:

In [changeset:"4b977a5d7283e7ca51288cc0ed0860e0004653ca" 4b977a5d]:
{{{#!CommitTicketReference repository=""
revision="4b977a5d7283e7ca51288cc0ed0860e0004653ca"
Fixed #35444 -- Added generic support for Aggregate.order_by.

This moves the behaviors of `order_by` used in Postgres aggregates into
the `Aggregate` class. This allows for creating aggregate functions that
support this behavior across all database engines. This is shown by
moving the `StringAgg` class into the shared `aggregates` module and
adding support for all databases. The Postgres `StringAgg` class is now
a thin wrapper on the new shared `StringAgg` class.

Thank you Simon Charette for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:19>

[Django]

#35444: Add generic support for order by to Aggregate
-------------------------------------+-------------------------------------
Reporter: Chris M | Owner: Chris M
Type: New feature | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: orderableaggmixin | Triage Stage: Ready for
aggregate | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"ec7f0bcf79dec7412d00d48e43c995a45b3b7b70" ec7f0bc]:
{{{#!CommitTicketReference repository=""
revision="ec7f0bcf79dec7412d00d48e43c995a45b3b7b70"
Refs #35444 -- Adjusted multi-args distinct aggregate test ordering
expectations.

Unless an explicit order_by is specified for the test the ordering of the
aggregation results is undefined.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35444#comment:21>