Opened 10 years ago

Closed 7 years ago

#24747 closed New feature (fixed)

Allow transforms in order_by

Reported by: Ben Buchwald Owned by: Matthew Wilkes
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: josh.smeaton@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
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 (16)

comment:1 by Josh Smeaton, 10 years ago

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 by Matthew Schinckel, 9 years ago

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 by Artur Smęt, 9 years ago

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 by Josh Smeaton, 9 years ago

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 8 years ago by Tim Graham (previous) (diff)

comment:5 by Matthew Wilkes, 7 years ago

Owner: changed from nobody to Matthew Wilkes
Status: newassigned

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

comment:6 by Tim Graham, 7 years ago

WIP PR from Matthew

comment:7 by Austin Roberts, 7 years ago

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 by Matthew Wilkes, 7 years ago

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 by Matthew Wilkes, 7 years ago

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 by Simon Charette, 7 years ago

Triage Stage: UnreviewedAccepted

comment:11 by Matthew Wilkes, 7 years ago

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 by Josh Smeaton, 7 years ago

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.

comment:13 by Matthew Wilkes, 7 years ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

I'm unsetting these markers as since Josh set them I've added more tests and notes in the documentation, so I think it's ready to be looked at again. I've also rebased against master and put the documentation changes in against 2.1.

comment:14 by Carlton Gibson, 7 years ago

I added a couple of small points on the PR.

I have a query whether we want to emphasise the new functionality a bit more in the docs (or release notes) but I'd accept a "No" on that quite happily.

Beyond that, if nobody has any particular suggestions they want incorporated, this looks good to go to me. (My thought would be to allow a few days for feedback. If none is coming advance to Ready for Checkin.)

comment:15 by Carlton Gibson, 7 years ago

Triage Stage: AcceptedReady for checkin

comment:16 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In 2162f09:

Fixed #24747 -- Allowed transforms in QuerySet.order_by() and distinct(*fields).

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