[Django] #35309: Remove Order by on models when prefetching by id

[Django]

#35309: Remove Order by on models when prefetching by id
-------------------------------------+-------------------------------------
Reporter: Laurent | Owner: nobody
Lyaudet |
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: 5.0
layer (models, ORM) |
Severity: Normal | Keywords: prefetch order_by
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Hello,

I don't know if the "bug" is still here with Django 5.
But on my version of Django, I have the following "bug" :
Assume you have the following code :

{{{#!python
class A(models.Model):
name = models.CharField(max_length=200)

class Meta:
ordering = ["name"]


class B(models.Model):
a = models.ForeignKey(A, related_name="bs", on_delete=models.CASCADE)


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)
b2 = B.objects.create(a=a2)
b3 = B.objects.create(a=a3)
b4 = B.objects.create(a=a4)
b5 = B.objects.create(a=a5)
b6 = B.objects.create(a=a6)
b7 = B.objects.create(a=a7)

bs = B.objects.all().prefetch_related("a")
}}}

The prefetch of as will use the order by and add useless charge on the DB
server.
There may be other cases than ForeignKey where the order by is useless.
But since OneToOne inherits from ForeignKey, I don't see anything else
right now.

Hence, I request this enhancement, please :)
#ClimateChangeBrake

Best regards,
Laurent Lyaudet
--
Ticket URL: <https://code.djangoproject.com/ticket/35309>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35309: Remove Order by on models when prefetching by id
-------------------------------------+-------------------------------------

Reporter: Laurent Lyaudet | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution: invalid

Keywords: prefetch order_by | Triage Stage:

| Unreviewed
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* resolution: => invalid
* status: new => closed

Comment:

`Meta.ordering` is working as expected, please
[https://docs.djangoproject.com/en/5.0/ref/models/options/#ordering refer
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.

{{{#!python
B.objects.prefetch_related(Prefetch("a", A.objects.order_by()))
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35309#comment:1>

[Django]

#35309: Remove Order by on models when prefetching by id
-------------------------------------+-------------------------------------
Reporter: Laurent Lyaudet | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: prefetch order_by | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Laurent Lyaudet):

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...
--
Ticket URL: <https://code.djangoproject.com/ticket/35309#comment:2>

[Django]

#35309: Remove Order by on models when prefetching by id
-------------------------------------+-------------------------------------
Reporter: Laurent Lyaudet | Owner: nobody

Type: | Status: new

Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:

Keywords: prefetch order_by | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Laurent Lyaudet):

* resolution: invalid =>
* status: closed => new

--
Ticket URL: <https://code.djangoproject.com/ticket/35309#comment:3>

[Django]

#35309: Remove Order by on models when prefetching by id
-------------------------------------+-------------------------------------
Reporter: Laurent Lyaudet | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: prefetch order_by | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35309#comment:4>

[Django]

#35309: Remove Order by on models when prefetching by id
-------------------------------------+-------------------------------------
Reporter: Laurent Lyaudet | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: prefetch order_by | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Laurent Lyaudet):

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:

{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35309#comment:5>

[Django]

#35309: Remove Order by on models when prefetching by id
-------------------------------------+-------------------------------------
Reporter: Laurent Lyaudet | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: prefetch order_by | Triage Stage:
| Unreviewed

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Laurent Lyaudet):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/35309#comment:6>

[Django]

#35309: Remove Order by on models when prefetching by id
-------------------------------------+-------------------------------------
Reporter: Laurent Lyaudet | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: prefetch order_by | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Laurent Lyaudet):

Here is the PR, I will improve it when requested :
https://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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35309#comment:7>

[Django]

#35309: Remove Order by on models when prefetching by id
-------------------------------------+-------------------------------------
Reporter: Laurent Lyaudet | Owner: nobody

Type: | Status: closed

Cleanup/optimization |
Component: Database layer | Version: 5.0
(models, ORM) |

Severity: Normal | Resolution: wontfix

Keywords: prefetch order_by | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* resolution: => wontfix

* status: new => closed

Comment:

Laurent, thanks for this patch, however I agree with Simon.

I appreciate you'd like to reopen the ticket, but please
[https://docs.djangoproject.com/en/stable/internals/contributing/triaging-
tickets/#closing-tickets follow the triaging guidelines with regards to
wontfix tickets] and take this to DevelopersMailingList.
--
Ticket URL: <https://code.djangoproject.com/ticket/35309#comment:8>