[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>