Opened 2 years ago

Last modified 2 months ago

#24747 assigned New feature

Allow transforms in order_by

Reported by: Ben Buchwald Owned by: Matthew Wilkes
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: josh.smeaton@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The extension of lookups to support transforms is great, but I'd like to be able to order by a transformed field.

As a use case, let's say I define a length transform. I filter by length such as Song.objects.filter(title__length__gt=10), but I think it makes sense that I ought to also be able to sort by this transform, for instance: Song.objects.all().order_by('title__length')

The related ticket #24629 points out that Func expressions should be usable as transforms. Although in 1.8 I could get the desired sort by ordering by the Length func expression, I feel that the reverse should also be true, that I should be able to order by a transform.

Change History (12)

comment:1 Changed 2 years ago by Josh Smeaton

Cc: josh.smeaton@… added
Triage Stage: UnreviewedAccepted
Version: 1.8master

This is the general problem that matches these specific tickets:

https://code.djangoproject.com/ticket/24592
https://code.djangoproject.com/ticket/23709

And copying my comment from 24592:

There are possibly two ways to fix this situation.

  1. Make all Transforms into Expressions. That is, we need to consolidate the very close APIs of Transforms/Lookups and Expressions. We have and we are discussing this elsewhere (though I can't find where just at the moment). This is the most likely solution we'll converge on.
  1. Make .values and .order_by understand the transformation/lookup API. We'll probably steer clear of this as it helps to widen the gap between transforms/expressions.

I now think we should implement both, although 2 will be dependent one 1 (where 1 is tracked here: https://code.djangoproject.com/ticket/24629). The idea is that transforms should be expressions, and then converting the transform syntax into the expression syntax internally would allow users to use whichever syntax they'd prefer.

I'll close the other tickets that are asking for specific transforms and leave this one open as it calls out the general issue.

comment:2 Changed 2 years ago by Matthew Schinckel

This also makes sense within the context of structured fields, like Array, JSON and Hstore: you may want to order by a lookup within the field.

I believe this will be addressed by this proposed solution.

comment:3 Changed 20 months ago by Artur Smęt

Also distinct by json/hstore field's key is not possible now. And it looks like, there is no workaround with Expressions because DISTINCT queries are not expressions as such.
Are there any plans, when this feature will be supported?

comment:4 Changed 20 months ago by Josh Smeaton

I had a quick look at this a few weeks back, and while I didn't actually implement any of it, I think I found the place where the work would need to be done.

https://github.com/jarshwah/django/commit/0522ccd1b4f7867f662de238ec9437dbed72f0d9

I would hope to get this done before django 1.10, but my time lately has been totally consumed by day-job and family.

As far as distinct queries go, distinct would have to support F() expressions internally for this patch to have any effect there. I'm not even certain that this would be enough, because I think you'd still need to have the field__transform in the select list. I haven't played with this enough to know though.

Last edited 17 months ago by Tim Graham (previous) (diff)

comment:5 Changed 4 months ago by Matthew Wilkes

Owner: changed from nobody to Matthew Wilkes
Status: newassigned

I'm having a go at this between talks at PyCon.

comment:6 Changed 4 months ago by Tim Graham

WIP PR from Matthew

comment:7 Changed 4 months ago by Austin Roberts

As I noted on #28168, I found a work around for this, though it would still be great to have this work the same way as filter.

For now, I'm able to work around this by instead doing:

from django.db.models.expressions import OrderBy, RawSQL

MyModel.objects.all().order_by(OrderBy(RawSQL("metadata #> '{some}' #> '{field}'", []))

Similarly, I have cases where I want to be able to do

MyModel.objects.all().values("metadata__some__field")

I've worked out a similar work around of

MyModel.objects.all().annotate(some_field=RawSQL("metadata #> '{some}' #> '{field}'", [])).values("some_field")

comment:8 Changed 4 months ago by Matthew Wilkes

The RawSQL trick is a nice workaround. For my part, I've done some work on this and have order_by, distinct and values working on JSON fields, as well as order_by on a toy lookup for testing purposes.

The PR isn't in a mergable state yet, as I've left lots of debugging help in for myself and not tidied the commits yet, but I'm open to ideas for additional things I should test.

Austin, do the tests at https://github.com/django/django/pull/8528/files#diff-7a482072a1e7be83b1c715879401a4f6 accurately represent what you want from JSON fields?

comment:9 Changed 3 months ago by Matthew Wilkes

Has patch: set
Triage Stage: AcceptedUnreviewed

I've updated this PR to have what I think is a good implementation of this problem. I'd appreciate review, especially from Josh as he started working on it.

comment:10 Changed 3 months ago by Simon Charette

Triage Stage: UnreviewedAccepted

comment:11 Changed 2 months ago by Matthew Wilkes

I know the depths of the ORM mean there's not many people who feel comfortable reviewing changes, but if anyone does have time to take a look and see if this needs more work I'd be very grateful. Happy to provide beer-based bribes, if that'll help.

comment:12 Changed 2 months ago by Josh Smeaton

Needs documentation: set
Needs tests: set
Patch needs improvement: set

A few things to tidy up or consider. Need some usage documentation. Need some tests outside contrib/postgres showcasing new abilities. Old docs might need to be changed to eliminate redundant calls like annotate().values().annotate().

I don't see anything fundamentally wrong though - it's very nearly ready for a merge I think. I'll know a lot more once I've actually used the changes.

Note: See TracTickets for help on using tickets.
Back to Top