Make the subquery ordering clearing optimization more selectively

[Hannes Ljungberg]

This will allow to add support for 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 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 ​before but it ended up being ​reverted due to some failures on Oracle. We'll need address these failures if they are still relevant.

[Hannes Ljungberg]

Messed up the CC:ing, sorry if anyone not related to this ticket got an email!

[Simon Charette]

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.

[Mariusz Felisiak]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In 053141d3:

Refs #32786 -- Made Query.clear_ordering() not to cause side effects by default.

[Mariusz Felisiak <felisiak.mariusz@…>]

In d8c90d4c:

Fixed #32786 -- Moved subquery ordering clearing optimization to the _in lookup.

Co-Authored-By: Simon Charette <charette.s@…>

[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?

[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 ​supported versions policy for more details.

[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.

[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 ;)

[Mariusz Felisiak <felisiak.mariusz@…>]

In c48b34e:

Refs #32786 -- Made query clear ordering when ordered combined queryset is used in subquery on Oracle.

[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

[Natalia Bidart]

Replying to 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