Repository: django/django
Issue category: Performance Issue

Issue:
Elide ordering of prefetch querysets for single valued relationships
Description
		
(last modified by Laurent Lyaudet)		
While the ordering of multi-valued relationships must be preserved when prefetching relationships is it unnecessary when using prefetch_related against single valued relationships.
For example, given the following models
class Author(models.Model):
	name = models.CharField(max_length=200)
	class Meta:
		ordering = ["name"]
class Book(models.Model):
	title = models.CharField(max_length=200)
	author = models.ForeignKey(Author, related_name="books", on_delete=models.CASCADE)
	class Meta:
		ordering = ["title"]
The ordering of an author's books in Author.objects.prefetch_related("books") has a significance as multiple books might be associated with each authors.
It's not the case for a book's author in Book.objects.prefetch_related("author") through as the relationship can only contain a single author and there is a single way to order the members of a singleton.
In other words sorted([element], key=sort_func) will result in [element] for any sort_func.
This property holds true for all the single valued relationships that the ORM supports (backward and forward 1:1 and forward 1:M) which allows the prefetching to elide any predefined ordering safely to avoid an unnecessary and possibly expensive ordering defined for the related model queryset.
Currently the prefetch of authors will use the order by and add useless load on the DB server.
It would be useful to remove this order by.
#ClimateChangeBrake


Hints:
['Meta.ordering is working as expected, please \u200brefer to its documentation and associated warning Ordering is not a free operation. Each field you add to the ordering incurs a cost to your database. Each foreign key you add will implicitly include all of its default orderings as well. If you don\'t want this implicit behaviour then don\'t use Meta.ordering. If you want to keep using it but not for particular prefetches than use Prefetch objects with a queryset that explicitly calls order_by() to disable ordering. B.objects.prefetch_related(Prefetch("a", A.objects.order_by()))', 1710530134.0]
["Again a fast and without thought answer. I already know for this solution with Prefetch. Continue bashing good ideas because you don't like people giving them. I'll applaude at the end. There is no way it is useful to keep an order by when you do a query SELECT * FROM a WHERE a.id IN (.....100 or more ids here) ORDER BY name; then add the result in the cache of B objects. What you reject without thought yields a speed-up of 10 to 15 % on very big prefetches...", 1710531280.0]
["Please refrain from assuming bad faith from triagers regarding the resolution of this ticket. The provided resolution was a reflected based on your report details and in no way based on your persona. What do you suggest should happen for the thousands of projects out there that rely on prefetch_related to return results in a way that respects Meta.ordering? We can't simply make the behaviour of prefetch_related inconsistent with the normal behaviour or related manager access because it performs poorly when defined against an non-indexed field. I think the documentation warning I referred to is unfortunately all we can do to warn about this behaviour. Either use Meta.ordering and be prepared to deal with its implicit footguns or don't use it and use order_by where appropriate. Whether Meta.ordering should exist in the first place is debatable as it's at the origin of many unexpected behaviour with other features of the ORM (aggregation comes to mind) but making prefetch_related special case it would not only be backward incompatible but inconsistent with how the rest of the framework treats it.", 1710538813.0]
['I spent my night on it but I was able to make a patch, and I don\'t think there will be any regression. Consider the following models in some project TestNoOrderByForForeignKeyPrefetches and some app test_no_order_by models.py file: from django.db import models class A(models.Model): name = models.CharField(max_length=200) class Meta: ordering = ["name"] class B(models.Model): name = models.CharField(max_length=200) a = models.ForeignKey(A, related_name="bs", on_delete=models.CASCADE) class Meta: ordering = ["name"] Then consider the following command TestNoOrderByForForeignKeyPrefetches/test_no_order_by/management/commands/test_no_order_by_command.py : from django.core.management.base import BaseCommand from django.db import connection from django.db.models import Prefetch, QuerySet, RawQuerySet from django.db.models.fields.related_descriptors import ( ForwardManyToOneDescriptor, ReverseOneToOneDescriptor, ) from TestNoOrderByForForeignKeyPrefetches.test_no_order_by.models import A, B old_prefetch_init = Prefetch.__init__ def new_prefetch_init(self, *args, **kwargs): result = old_prefetch_init(self, *args, **kwargs) if self.queryset is not None: self.queryset._do_not_modify_order_by = True return result Prefetch.__init__ = new_prefetch_init old_get_prefetch_querysets_forward_many_to_one = ForwardManyToOneDescriptor.get_prefetch_querysets old_get_prefetch_querysets_reverse_one_to_one = ReverseOneToOneDescriptor.get_prefetch_querysets def get_prefetch_querysets_forward_many_to_one(self, *args, **kwargs): result = old_get_prefetch_querysets_forward_many_to_one(self, *args, **kwargs) if not hasattr(result[0], \'_do_not_modify_order_by\'): result = (result[0].order_by(), *result[1:]) return result def get_prefetch_querysets_reverse_one_to_one(self, *args, **kwargs): result = old_get_prefetch_querysets_reverse_one_to_one(self, *args, **kwargs) if not hasattr(result[0], \'_do_not_modify_order_by\'): result = (result[0].order_by(), *result[1:]) return result ForwardManyToOneDescriptor.get_prefetch_querysets = get_prefetch_querysets_forward_many_to_one ReverseOneToOneDescriptor.get_prefetch_querysets = get_prefetch_querysets_reverse_one_to_one old_clone_queryset = QuerySet._clone def new_clone_queryset(self): result = old_clone_queryset(self) if hasattr(self, \'_do_not_modify_order_by\'): result._do_not_modify_order_by = True return result QuerySet._clone = new_clone_queryset old_clone_raw_queryset = RawQuerySet._clone def new_clone_raw_queryset(self): result = old_clone_raw_queryset(self) if hasattr(self, \'_do_not_modify_order_by\'): result._do_not_modify_order_by = True return result RawQuerySet._clone = new_clone_raw_queryset class Command(BaseCommand): help = "Test" def handle(self, *args, **options): B.objects.all().delete() A.objects.all().delete() a1 = A.objects.create(name="a1") a2 = A.objects.create(name="a2") a3 = A.objects.create(name="a3") a4 = A.objects.create(name="a4") a5 = A.objects.create(name="a5") a6 = A.objects.create(name="a6") a7 = A.objects.create(name="a7") b1 = B.objects.create(a=a1, name="b1") b2 = B.objects.create(a=a2, name="b2") b3 = B.objects.create(a=a3, name="b3") b4 = B.objects.create(a=a4, name="b4") b5 = B.objects.create(a=a5, name="b5") b6 = B.objects.create(a=a6, name="b6") b7 = B.objects.create(a=a7, name="b7") bs = list(B.objects.all().prefetch_related("a")) a_s = list(A.objects.all().prefetch_related("bs")) bs = list(B.objects.all().prefetch_related( Prefetch( "a", queryset=A.objects.order_by("-name") ), )) a_s = list(A.objects.all().prefetch_related( Prefetch( "bs", queryset=B.objects.order_by("-name") ), )) print(connection.queries) If you launch the command with python3 manage.py test_no_order_by_command, you will see that there are 8 SELECT after the 14 INSERT and that there is only 7 ORDER BY on them as requested. I will prepare a PR.', 1710549223.0]
['\u200bPR', 1710551387.0]
['Here is the PR, I will improve it when requested : \u200bhttps://github.com/django/django/pull/17984 :) I still have doubts about keeping the order by even with manual Prefetch. I need to verify if it is possible to bypass the filter by id.', 1710555004.0]
["Laurent, thanks for this patch, however I agree with Simon. I appreciate you'd like to reopen the ticket, but please \u200bfollow the triaging guidelines with regards to wontfix tickets and take this to DevelopersMailingList.", 1710567356.0]

Patch:
diff --git a/django/db/models/fields/related_descriptors.py b/django/db/models/fields/related_descriptors.py
index a2f00eb172c4..c7848ee63a3e 100644
--- a/django/db/models/fields/related_descriptors.py
+++ b/django/db/models/fields/related_descriptors.py
@@ -195,6 +195,9 @@ def get_prefetch_querysets(self, instances, querysets=None):
         else:
             query = {"%s__in" % self.field.related_query_name(): instances}
         queryset = queryset.filter(**query)
+        # There can be only one object prefetched for each instance so clear
+        # ordering if the query allows it without side effects.
+        queryset.query.clear_ordering()
 
         # Since we're going to assign directly in the cache,
         # we must manage the reverse relation cache manually.
@@ -469,6 +472,9 @@ def get_prefetch_querysets(self, instances, querysets=None):
         instances_dict = {instance_attr(inst): inst for inst in instances}
         query = {"%s__in" % self.related.field.name: instances}
         queryset = queryset.filter(**query)
+        # There can be only one object prefetched for each instance so clear
+        # ordering if the query allows it without side effects.
+        queryset.query.clear_ordering()
 
         # Since we're going to assign directly in the cache,
         # we must manage the reverse relation cache manually.


Test patch:
diff --git a/tests/prefetch_related/test_prefetch_related_objects.py b/tests/prefetch_related/test_prefetch_related_objects.py
index ca1f904c5205..eea9a7fff78c 100644
--- a/tests/prefetch_related/test_prefetch_related_objects.py
+++ b/tests/prefetch_related/test_prefetch_related_objects.py
@@ -1,7 +1,7 @@
 from django.db.models import Prefetch, prefetch_related_objects
 from django.test import TestCase
 
-from .models import Author, Book, Reader
+from .models import Author, Book, House, Reader, Room
 
 
 class PrefetchRelatedObjectsTests(TestCase):
@@ -33,6 +33,17 @@ def setUpTestData(cls):
         cls.reader1.books_read.add(cls.book1, cls.book4)
         cls.reader2.books_read.add(cls.book2, cls.book4)
 
+        cls.house1 = House.objects.create(name="b1", address="1")
+        cls.house2 = House.objects.create(name="b2", address="2")
+
+        cls.room1 = Room.objects.create(name="a1", house=cls.house1)
+        cls.room2 = Room.objects.create(name="a2", house=cls.house2)
+
+        cls.house1.main_room = cls.room1
+        cls.house1.save()
+        cls.house2.main_room = cls.room2
+        cls.house2.save()
+
     def test_unknown(self):
         book1 = Book.objects.get(id=self.book1.id)
         with self.assertRaises(AttributeError):
@@ -58,20 +69,75 @@ def test_m2m_reverse(self):
 
     def test_foreignkey_forward(self):
         authors = list(Author.objects.all())
-        with self.assertNumQueries(1):
+        with self.assertNumQueries(1) as ctx:
             prefetch_related_objects(authors, "first_book")
+        self.assertNotIn("ORDER BY", ctx.captured_queries[0]["sql"])
 
         with self.assertNumQueries(0):
             [author.first_book for author in authors]
 
+        authors = list(Author.objects.all())
+        with self.assertNumQueries(1) as ctx:
+            prefetch_related_objects(
+                authors,
+                Prefetch("first_book", queryset=Book.objects.order_by("-title")),
+            )
+        self.assertNotIn("ORDER BY", ctx.captured_queries[0]["sql"])
+
     def test_foreignkey_reverse(self):
         books = list(Book.objects.all())
-        with self.assertNumQueries(1):
+        with self.assertNumQueries(1) as ctx:
             prefetch_related_objects(books, "first_time_authors")
+        self.assertIn("ORDER BY", ctx.captured_queries[0]["sql"])
 
         with self.assertNumQueries(0):
             [list(book.first_time_authors.all()) for book in books]
 
+        books = list(Book.objects.all())
+        with self.assertNumQueries(1) as ctx:
+            prefetch_related_objects(
+                books,
+                Prefetch(
+                    "first_time_authors",
+                    queryset=Author.objects.order_by("-name"),
+                ),
+            )
+        self.assertIn("ORDER BY", ctx.captured_queries[0]["sql"])
+
+    def test_one_to_one_forward(self):
+        houses = list(House.objects.all())
+        with self.assertNumQueries(1) as ctx:
+            prefetch_related_objects(houses, "main_room")
+        self.assertNotIn("ORDER BY", ctx.captured_queries[0]["sql"])
+
+        with self.assertNumQueries(0):
+            [house.main_room for house in houses]
+
+        houses = list(House.objects.all())
+        with self.assertNumQueries(1) as ctx:
+            prefetch_related_objects(
+                houses,
+                Prefetch("main_room", queryset=Room.objects.order_by("-name")),
+            )
+        self.assertNotIn("ORDER BY", ctx.captured_queries[0]["sql"])
+
+    def test_one_to_one_reverse(self):
+        rooms = list(Room.objects.all())
+        with self.assertNumQueries(1) as ctx:
+            prefetch_related_objects(rooms, "main_room_of")
+        self.assertNotIn("ORDER BY", ctx.captured_queries[0]["sql"])
+
+        with self.assertNumQueries(0):
+            [room.main_room_of for room in rooms]
+
+        rooms = list(Room.objects.all())
+        with self.assertNumQueries(1) as ctx:
+            prefetch_related_objects(
+                rooms,
+                Prefetch("main_room_of", queryset=House.objects.order_by("-name")),
+            )
+        self.assertNotIn("ORDER BY", ctx.captured_queries[0]["sql"])
+
     def test_m2m_then_m2m(self):
         """A m2m can be followed through another m2m."""
         authors = list(Author.objects.all())