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

[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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018e46d52203-b962cbd6-e7a0-4771-b2cb-3d4289588dd4-00%40eu-central-1.amazonses.com.

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

[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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018e4618a810-2a3b464f-cb6d-4f53-a5fd-e860d41b382b-00%40eu-central-1.amazonses.com.

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

[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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018e45e1768a-107e903a-054f-42be-ae93-a96f808a-00%40eu-central-1.amazonses.com.

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

[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: 
Django 
The 

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

[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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018e45219a8c-4cf523eb-82b6-49f8-828a-923a60e82a0b-00%40eu-central-1.amazonses.com.

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

[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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018e44af63f1-9cef1a2c-b83c-4cc4-8727-47ee8a66d872-00%40eu-central-1.amazonses.com.

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

[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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018e44aea931-c52f5858-1587-478c-bac0-3dc851f82f5b-00%40eu-central-1.amazonses.com.

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

[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: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/0107018e449d29fd-40c001e8-74dd-4a3a-924e-0a2a190708ae-00%40eu-central-1.amazonses.com.