Fixed #19195 -- Allow explicit ordering by a relation source field. #2614

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Member

charettes commented Apr 28, 2014

Thanks to chrisedgemon for the report and akaariai for the review.

@shaib shaib commented on an outdated diff Apr 28, 2014

django/db/models/sql/compiler.py
@@ -464,8 +464,9 @@ def find_ordering_name(self, name, opts, alias=None, default_order='ASC',
field, targets, alias, joins, path, opts = self._setup_joins(pieces, opts, alias)
# If we get to this point and the field is a relation to another model,
- # append the default ordering for that model.
- if field.rel and path and opts.ordering:
+ # append the default ordering for that model unless the attribute name
+ # of the field is specified.
+ if field.rel and path and opts.ordering and not name == field.attname:
@shaib

shaib Apr 28, 2014

Member

not name == field.attname => name != field.attname ?

@shaib shaib commented on an outdated diff Apr 28, 2014

docs/ref/models/querysets.txt
@@ -294,6 +294,17 @@ primary key if there is no :attr:`Meta.ordering
...since the ``Blog`` model has no default ordering specified.
+.. versionadded:: 1.7
+ Note that is also possible to order a queryset by the relation source
@shaib

shaib Apr 28, 2014

Member

Note that it is also

@shaib shaib commented on an outdated diff Apr 28, 2014

docs/ref/models/querysets.txt
@@ -294,6 +294,17 @@ primary key if there is no :attr:`Meta.ordering
...since the ``Blog`` model has no default ordering specified.
+.. versionadded:: 1.7
+ Note that is also possible to order a queryset by the relation source
+ field in order to avoid an extraneous JOIN to the foreign table.
+ For example, as long as your database backend enforce foreign key
@shaib

shaib Apr 28, 2014

Member

...backend enforce_s_ (unless you're using a subjunctive form ;-)

@shaib shaib commented on an outdated diff Apr 28, 2014

docs/ref/models/querysets.txt
@@ -294,6 +294,17 @@ primary key if there is no :attr:`Meta.ordering
...since the ``Blog`` model has no default ordering specified.
+.. versionadded:: 1.7
+ Note that is also possible to order a queryset by the relation source
+ field in order to avoid an extraneous JOIN to the foreign table.
+ For example, as long as your database backend enforce foreign key
+ constraints::
+
+ Entry.objects.order_by('blog_id')
@shaib

shaib Apr 28, 2014

Member

Add a note pointing out the difference between blog__id above and blog_id here. I missed it at first reading.

@shaib shaib commented on an outdated diff Apr 28, 2014

docs/releases/1.7.txt
@@ -702,6 +702,9 @@ Models
Previously model field validation didn't prevent values out of their associated
column data type range from being saved resulting in an integrity error.
+* It is now possible to explicitely :meth:`order_by() <django.db.models.query.QuerySet.order_by>`
@shaib

shaib Apr 28, 2014

Member

explicitly (redundant e), I think.

@shaib shaib commented on an outdated diff Apr 28, 2014

docs/releases/1.7.txt
@@ -702,6 +702,9 @@ Models
Previously model field validation didn't prevent values out of their associated
column data type range from being saved resulting in an integrity error.
+* It is now possible to explicitely :meth:`order_by() <django.db.models.query.QuerySet.order_by>`
+ a relation source field by using its ``attname``.
@shaib

shaib Apr 28, 2014

Member

I think "attribute name" is preferable to attname

Member

charettes commented Apr 29, 2014

Update PR based on @shaib input. Thanks!

@jarshwah jarshwah commented on an outdated diff Apr 29, 2014

docs/ref/models/querysets.txt
@@ -294,6 +294,19 @@ primary key if there is no :attr:`Meta.ordering
...since the ``Blog`` model has no default ordering specified.
+.. versionadded:: 1.7
+
+ Note that it is also possible to order a queryset by the relation source
@jarshwah

jarshwah Apr 29, 2014

Member

I found this whole section quite a mouth full, and it took me a few reads to "get it". I think it could be simplified by leaving out some detail that could be implied. Something like the below perhaps?

"Note that it is also possible to order a queryset by a related field, without incurring the cost of a JOIN, by referring to the id of the related field::

# No Join
Entry.objects.order_by('blog_id')

# Join
Entry.objects.order_by('blog')

@jarshwah jarshwah and 2 others commented on an outdated diff Apr 29, 2014

docs/releases/1.7.txt
@@ -702,6 +702,9 @@ Models
Previously model field validation didn't prevent values out of their associated
column data type range from being saved resulting in an integrity error.
+* It is now possible to explicitly :meth:`order_by() <django.db.models.query.QuerySet.order_by>`
+ a relation source field by using its attribute name.
@jarshwah

jarshwah Apr 29, 2014

Member

"relation source field" is too technical I believe. I don't think many people reading these notes will get that relatation_id will now work. Perhaps a better name for "relation source field" that is standardised across the docs would be more appropriate. What that name could be, I don't know.

@charettes

charettes Apr 29, 2014

Member

What about the relation local field?

@jarshwah

jarshwah Apr 30, 2014

Member

I don't think that really describes it either without knowing that a relation has a local pointer. I'm being pedantic, so unless someone else has a better suggestion you should probably just leave it as is.

The django docs explain (briefly) about the database representation of foreign keys, but never really mention the local source field (https://docs.djangoproject.com/en/1.6/ref/models/fields/#database-representation). Perhaps if it was mentioned there and linked to from the release notes (order_by now respects a foreign keys ('local source field')_ )? But that's probably going beyond what this patch is attempting anyway.

@timgraham

timgraham Apr 30, 2014

Owner

Agreed it's confusing; is "the relation's _id field" accurate?

@charettes

charettes Apr 30, 2014

Member

I think _id field makes sense here, thanks!

@charettes charettes and 2 others commented on an outdated diff Apr 29, 2014

docs/ref/models/querysets.txt
@@ -294,6 +294,18 @@ primary key if there is no :attr:`Meta.ordering
...since the ``Blog`` model has no default ordering specified.
+.. versionadded:: 1.7
+
+ Note that it is also possible to order a queryset by a related field,
+ without incurring the cost of a JOIN, by referring to the ``id`` of the
+ related field::
+
+ # No Join
+ Entry.objects.order_by('blog_id')
+
+ # Join
+ Entry.objects.order_by('blog__id')
@charettes

charettes Apr 29, 2014

Member

@jarshwah, amended this part of the documentation but I'm still not sure about the "id of the related field" wording.

@jarshwah

jarshwah Apr 30, 2014

Member

Neither am I, but I find this more clear. Perhaps someone else can suggest a touch up? And maybe order_by('blog__id') should be order_by('blog') to make the distinction more clear.

@timgraham

timgraham Apr 30, 2014

Owner

Current wording is okay with me.

@timgraham timgraham commented on an outdated diff Apr 30, 2014

docs/ref/models/querysets.txt
@@ -294,6 +294,18 @@ primary key if there is no :attr:`Meta.ordering
...since the ``Blog`` model has no default ordering specified.
+.. versionadded:: 1.7
+
+ Note that it is also possible to order a queryset by a related field,
+ without incurring the cost of a JOIN, by referring to the ``id`` of the
+ related field::
+
+ # No Join
@timgraham

timgraham Apr 30, 2014

Owner

This code block needs to be indented (doc build error otherwise).

@timgraham timgraham commented on an outdated diff Apr 30, 2014

docs/ref/models/querysets.txt
@@ -435,6 +447,20 @@ Examples (those after the first will only work on PostgreSQL)::
>>> Entry.objects.order_by('author', 'pub_date').distinct('author')
[...]
+.. note::
+ Keep in mind that :meth:`order_by` uses the default related model
@timgraham

timgraham Apr 30, 2014

Owner

I would say "uses any default related model ordering that has been defined"

@timgraham timgraham commented on an outdated diff Apr 30, 2014

docs/ref/models/querysets.txt
@@ -435,6 +447,20 @@ Examples (those after the first will only work on PostgreSQL)::
>>> Entry.objects.order_by('author', 'pub_date').distinct('author')
[...]
+.. note::
+ Keep in mind that :meth:`order_by` uses the default related model
+ ordering if it has been defined. You might have to explicitly order
+ by the relation source or referenced field to make sure ``DISTINCT ON``
+ expressions match initial ``ORDER BY`` ones. For example, in the case
+ where the ``Blog`` model defined an ``ordering`` by ``name``::
@timgraham

timgraham Apr 30, 2014

Owner

add link: :attr:django.db.models.Options.ordering

@timgraham timgraham commented on an outdated diff Apr 30, 2014

docs/ref/models/querysets.txt
@@ -435,6 +447,20 @@ Examples (those after the first will only work on PostgreSQL)::
>>> Entry.objects.order_by('author', 'pub_date').distinct('author')
[...]
+.. note::
+ Keep in mind that :meth:`order_by` uses the default related model
+ ordering if it has been defined. You might have to explicitly order
+ by the relation source or referenced field to make sure ``DISTINCT ON``
+ expressions match initial ``ORDER BY`` ones. For example, in the case
@timgraham

timgraham Apr 30, 2014

Owner

"in the case where" -> "if the"

@timgraham timgraham and 1 other commented on an outdated diff Apr 30, 2014

docs/ref/models/querysets.txt
@@ -435,6 +447,20 @@ Examples (those after the first will only work on PostgreSQL)::
>>> Entry.objects.order_by('author', 'pub_date').distinct('author')
[...]
+.. note::
+ Keep in mind that :meth:`order_by` uses the default related model
+ ordering if it has been defined. You might have to explicitly order
+ by the relation source or referenced field to make sure ``DISTINCT ON``
@timgraham

timgraham Apr 30, 2014

Owner

Unclear what "relation source or referenced field" means.
add "the": the DISTINCT ON expressions match the ORDER BY ones
can "initial" be removed? not sure what it means

@charettes

charettes Apr 30, 2014

Member

Unclear what "relation source or referenced field" means.

Changing to _id as suggested below.

can "initial" be removed? not sure what it means

Initial means: "... that the fields in order_by() must start with the fields in distinct(), in the same order."

@timgraham

timgraham Apr 30, 2014

Owner

gotcha, how about: "make sure the DISTINCT ON expressions match those at the beginning of the ORDER BY clause"

@charettes

charettes Apr 30, 2014

Member

Good for me!

@timgraham timgraham commented on an outdated diff Apr 30, 2014

docs/ref/models/querysets.txt
@@ -435,6 +447,20 @@ Examples (those after the first will only work on PostgreSQL)::
>>> Entry.objects.order_by('author', 'pub_date').distinct('author')
[...]
+.. note::
+ Keep in mind that :meth:`order_by` uses the default related model
+ ordering if it has been defined. You might have to explicitly order
+ by the relation source or referenced field to make sure ``DISTINCT ON``
+ expressions match initial ``ORDER BY`` ones. For example, in the case
+ where the ``Blog`` model defined an ``ordering`` by ``name``::
+
+ Entry.objects.order_by('blog').distinct('blog')
+
+ ...wouldn't work because the query would be ordered by ``blog__name`` thus
+ mismatching the ``DISTINCT ON`` expression. You'd have to explicitly order
+ by the relation source field (``blog_id`` in this case) or the referenced
+ one (``blog__pk``) to make sure both expresions match.
@timgraham

timgraham Apr 30, 2014

Owner

expressions*

@timgraham timgraham and 1 other commented on an outdated diff Apr 30, 2014

docs/ref/models/querysets.txt
@@ -435,6 +447,20 @@ Examples (those after the first will only work on PostgreSQL)::
>>> Entry.objects.order_by('author', 'pub_date').distinct('author')
[...]
+.. note::
+ Keep in mind that :meth:`order_by` uses the default related model
+ ordering if it has been defined. You might have to explicitly order
+ by the relation source or referenced field to make sure ``DISTINCT ON``
+ expressions match initial ``ORDER BY`` ones. For example, in the case
+ where the ``Blog`` model defined an ``ordering`` by ``name``::
+
+ Entry.objects.order_by('blog').distinct('blog')
+
+ ...wouldn't work because the query would be ordered by ``blog__name`` thus
+ mismatching the ``DISTINCT ON`` expression. You'd have to explicitly order
+ by the relation source field (``blog_id`` in this case) or the referenced
@timgraham

timgraham Apr 30, 2014

Owner

how about "the relation's _id field (blog_id) or its primary key (blog__pk)

@timgraham timgraham and 1 other commented on an outdated diff Apr 30, 2014

docs/releases/1.7.txt
@@ -702,6 +702,9 @@ Models
Previously model field validation didn't prevent values out of their associated
column data type range from being saved resulting in an integrity error.
+* It is now possible to explicitly :meth:`order_by() <django.db.models.query.QuerySet.order_by>`
@timgraham

timgraham Apr 30, 2014

Owner

you can split this line after order_by(), but why not use ~django.db.models.query.QuerySet.order_by instead?

@charettes

charettes Apr 30, 2014

Member

Right, I'll use ~django.db.models.query.QuerySet.order_by instead and won't have to split.

@charettes charettes Fixed #19195 -- Allow explicit ordering by a relation `_id` field.
Thanks to chrisedgemon for the report and shaib, akaariai and
timgraham for the review.
c57b9fc
Member

charettes commented Apr 30, 2014

Sorry @jarshwah, I forgot to mention you in the commit message...

Merged in 24ec953 and backported to 1.7.x in a6ecd5d.

charettes closed this Apr 30, 2014

charettes deleted the charettes:ticket-19195-fk-order_by-attname branch Aug 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment