[Django] #32786: Make the subquery ordering clearing optimization more selectively
Django
-------------------------------------+-------------------------------------
Reporter: Hannes | Owner: Hannes Ljungberg
Ljungberg |
Type: | Status: assigned
Cleanup/optimization |
Component: Database | Version: dev
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
This will allow to add support for
[https://code.djangoproject.com/ticket/32776 Array subqueries on
PostgreSQL]. It appears like we would only have to adapt `In.process_rhs`
to do the clearing when the `rhs` value is an instance of `Query` and
fullfils the requirements of clearing the ordering. As
[https://code.djangoproject.com/ticket/32776#comment:1 Simon commented in
#32776] about this we would also want to keep clearing ordering for
`Exists` and and `Queryset.exists()` and since these both end up running
`Query.exists()` which runs `clear_ordering` we should be fine.
I looked around historically and found out that there've been an attempt
at this [https://github.com/django/django/pull/8417 before] but it ended
up being [https://github.com/django/django/pull/8433 reverted] due to some
failures on Oracle. We'll need address these failures if they are still
relevant.
--
Ticket URL: <https://code.djangoproject.com/ticket/32786>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Hannes Ljungberg):
Messed up the CC:ing, sorry if anyone not related to this ticket got an
email!
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon, Charette (removed)
* cc: Simon Charette (added)
Comment:
> I looked around historically and found out that there've been an attempt
at this before but it ended up being reverted due to some failures on
Oracle. We'll need address these failures if they are still relevant.
Ah right I remember that, I'm curious to see what exactly it was breaking
on Oracle again.
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
[https://github.com/django/django/pull/14448 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Component: Database layer | Version: dev
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d8c90d4c22cb218f1c170eba086c53d3dff7fbc0" d8c90d4c]:
{{{
#!CommitTicketReference repository=""
revision="d8c90d4c22cb218f1c170eba086c53d3dff7fbc0"
Fixed #32786 -- Moved subquery ordering clearing optimization to the _in
lookup.
Co-Authored-By: Simon Charette <chare...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"053141d31fe5aef1c255a1be183383860e0ccce9" 053141d3]:
{{{
#!CommitTicketReference repository=""
revision="053141d31fe5aef1c255a1be183383860e0ccce9"
Refs #32786 -- Made Query.clear_ordering() not to cause side effects by
default.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Component: Database layer | Version: dev
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Andi Albrecht):
Do I may bring up this issue again? I've stumbled upon a similar issue as
reported in #32658 or #32786. Both were closed as invalid. However, this
change added a test case to Django's main branch (which will be 4.0 if I'm
not wrong) that tests a scenario that’s pretty similar to the code in the
application I’m migrating from 2.2 LTS to 3.2 LTS. The test case looks
totally legit to me and not like an abuse of the ORM:
{{{
def test_ordering_isnt_cleared_for_array_subquery(self):
inner_qs = AggregateTestModel.objects.order_by('-integer_field')
qs = AggregateTestModel.objects.annotate(
integers=Func(
Subquery(inner_qs.values('integer_field')),
function='ARRAY',
output_field=ArrayField(base_field=IntegerField()),
),
)
self.assertSequenceEqual(
qs.first().integers,
inner_qs.values_list('integer_field', flat=True),
)
}}}
When adding this test case to the Django 2.2 code base, the test passes.
When adding it to 3.2 it fails due to the removal of ordering. Of course
in main it passes too. So to me this looks like a regression in 3.2.
I did a quick test and cherry-picked both changes form this ticket in my
local 3.2 branch without much issues. Six tests in postgres_tests were
failing because of different warnings, but the same six tests fail on a
clean 3.2 branch on my machine too.
Wouldn’t it be an option to backport these changes to 3.2 to have a
consistent behavior in 2.2, 3.2 and the upcoming 4.x release?
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
> So to me this looks like a regression in 3.2.
This behavior was changed in Django 3.0 so per our backporting policy this
means it doesn't qualify for a backport to 3.2.x, even if we consider it a
regression.
See [https://docs.djangoproject.com/en/dev/internals/release-process
/#supported-versions supported versions policy] for more details.
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:10>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Hannes Ljungberg):
A workaround until 4.0 is available is to add a high limit to the query,
like `qs[:99999]`. This will prevent the ordering from being cleared.
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:11>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Andi Albrecht):
Ok, thanks for clarification, Mariusz! So we'll go with this high limit
workaround for a few years since we settle on LTS versions before we get
the old behavior back ;)
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:12>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"c48b34e26d0fd2f445d54d191bfe32d58dfb8e7b" c48b34e]:
{{{
#!CommitTicketReference repository=""
revision="c48b34e26d0fd2f445d54d191bfe32d58dfb8e7b"
Refs #32786 -- Made query clear ordering when ordered combined queryset is
used in subquery on Oracle.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:13>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by asottile-sentry):
can this change be added to the changelog? specifically this code from
django 3.2.x no longer succeeds:
without a deprecation period or a changelog entry I'm not certain what
this _should_ be changed to. the commit message also doesn't help me much
either
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:14>
Django
-------------------------------------+-------------------------------------
Type: | Ljungberg
Cleanup/optimization | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Natalia Bidart):
Replying to [comment:14 anthony sottile]:
> can this change be added to the changelog? specifically this code from
django 3.2.x no longer succeeds:
>
>
https://github.com/getsentry/sentry/blob/704da8578f2dc96afdf7e26ac29fb0f2b9576269/src/sentry/api/paginator.py#L187
>
> without a deprecation period or a changelog entry I'm not certain what
this _should_ be changed to. the commit message also doesn't help me much
either
I guess is too late for the deprecation path, as per the workaround for
3.2, I believe that's described in comment 11 in this ticket:
ticket:32786#comment:11
--
Ticket URL: <https://code.djangoproject.com/ticket/32786#comment:15>