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 , 10 years ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
Version: | 1.8 → master |
comment:2 by , 10 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 , 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 , 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.
comment:5 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
I'm having a go at this between talks at PyCon.
comment:7 by , 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 , 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 , 7 years ago
Has patch: | set |
---|---|
Triage Stage: | Accepted → Unreviewed |
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 , 7 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:11 by , 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 , 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 , 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 , 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 , 7 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
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.
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.