#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 <https://code.djangoproject.com/>
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/010701907fdcd1fd-36c4d3f4-5c38-4712-a1b7-f5cf3eca7f5b-000000%40eu-central-1.amazonses.com.
