Re: [Django] #35309: Elide ordering of prefetch querysets for single valued relationships

[Django]

#35309: Elide ordering of prefetch querysets for single valued relationships
-------------------------------------+-------------------------------------
Reporter: Laurent Lyaudet | Owner: Laurent
Type: | Lyaudet
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: prefetch single- | Triage Stage: Accepted
valued order_by |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):

* owner: nobody => Laurent Lyaudet
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/35309#comment:10>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35309: Elide ordering of prefetch querysets for single valued relationships
-------------------------------------+-------------------------------------
Reporter: Laurent Lyaudet | Owner: Laurent
Type: | Lyaudet
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: prefetch single- | Triage Stage: Accepted
valued order_by |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Laurent Lyaudet):

Hello,


Thanks for the replies and the constructive suggested improvements.


I removed `_do_not_modify_order_by`.


I improved to avoid an useless cloning of `QuerySet`.
For that I created a protected method
`QuerySet._in_place_empty_order_by()` just below `QuerySet.order_by()`.
I thought it would be the best solution without modifying the "public API"
of `QuerySet`.
Another solution could be to add kwargs to `QuerySet.order_by()` like :
- `clone: bool = True`, or `no_clone: bool = False`, or `in_place: bool =
False`,
- and maybe `clear_default: bool = False`.
But it would require a larger discussion I think.
If you think the name of `QuerySet._in_place_empty_order_by()` should be
changed for something else, tell me.


Regarding the tests, thanks for the suggestions.
I applied them in my two tests.
I think this is better practice to have two separate tests where the
intent of each test is clear.
And I haven't added the asserts in other tests.
However, I understand that you may want to avoid cluttering the tests and
the models in `tests/prefetch_related/models.py`.
I have see that I can replace the class `A35309` with the class `Book` and
the class `B35309` with the class `Author` in my first test.
Tell me if you think I should do that.
I have see that I can replace the class `A35309` with the class `Room` and
the class `C35309` with the class `House` in my second test.
Tell me if you think I should do that.

Best regards,
Laurent Lyaudet
--
Ticket URL: <https://code.djangoproject.com/ticket/35309#comment:11>

[Django]

#35309: Elide ordering of prefetch querysets for single valued relationships
-------------------------------------+-------------------------------------
Reporter: Laurent Lyaudet | Owner: Laurent
Type: | Lyaudet
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: prefetch single- | Triage Stage: Accepted
valued order_by |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Laurent Lyaudet:

Old description:

> 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
>
> {{{#!python
> 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.

New description:

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

{{{#!python
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
charge on the DB server.
It would be useful to remove this order by.
#ClimateChangeBrake

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

[Django]

load on the DB server.

It would be useful to remove this order by.
#ClimateChangeBrake

--
--

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

[Django]

#35309: Elide ordering of prefetch querysets for single valued relationships
-------------------------------------+-------------------------------------
Reporter: Laurent Lyaudet | Owner: Laurent
Type: | Lyaudet
Cleanup/optimization | Status: assigned
Component: Database layer | Version: 5.0
(models, ORM) |
Severity: Normal | Resolution:

Keywords: prefetch single- | Triage Stage: Ready for
valued order_by | checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin

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

[Django]

#35309: Elide ordering of prefetch querysets for single valued relationships
-------------------------------------+-------------------------------------
Reporter: Laurent Lyaudet | Owner: Laurent
Type: | Lyaudet

Cleanup/optimization | Status: closed

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

Severity: Normal | Resolution: fixed

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

Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"f2388a4b73ee74bd077854798a0ac1669d037304" f2388a4]:
{{{#!CommitTicketReference repository=""
revision="f2388a4b73ee74bd077854798a0ac1669d037304"
Fixed #35309 -- Made prefetch clear ordering for single-valued
relationships.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35309#comment:15>