Ambiguous aliases in ordering on combined queries with select_related().

[Shai Berger]

Ordering on combined queries with select_related() seems to have been slightly broken in [c58a8acd413ccc].

Consider these tests:

    # Model Author has  `ordering = ["name"]` in its Meta

    def test_union_with_select_related_and_order(self):
        base_qs = Author.objects.select_related('extra').order_by()
        qs1 = base_qs.filter(name="a1")
        qs2 = base_qs.filter(name="a2")
        self.assertFalse(
            bool(
                qs1.union(qs2).order_by("pk")
            )
        )

    # first() also calls order_by("pk")

    def test_union_with_select_related_and_first(self):
        base_qs = Author.objects.select_related('extra')
        qs1 = base_qs.filter(name="a1")
        qs2 = base_qs.filter(name="a2")
        self.assertFalse(
            bool(
                qs1.union(qs2).first()
            )
        )

On Postrgres, both of these pass before the named commit, and error after it with

django.db.utils.ProgrammingError: ORDER BY "id" is ambiguous
LINE 1: ...id") WHERE "queries_author"."name" = 'a2') ORDER BY "id" ASC

On Sqlite, the second test (where ordering from the model is in force) breaks even before this, with

django.db.utils.DatabaseError: ORDER BY not allowed in subqueries of compound statements.

but on current main, the first breaks too -- this time, with

django.db.utils.OperationalError: 1st ORDER BY term does not match any column in the result set

Note that on union queries without select_related(), ordering on pk works -- many existing tests use it, as well as another test I added (in the attached diff) just to be sure.

[Shai Berger]

Tests to show and investigate the regression

[Mariusz Felisiak]

Thanks for the report!

Regression in c58a8acd413ccc992dd30afd98ed900897e1f719.

[David Sanders]

Interesting read following the thread back.

I'm guessing reverting to using column numbers is out of the question, is the solution here to simply alias the columns c1, c2, c3 … etc. You only need to alias the first query in the union and the others follow suite.

[Simon Charette]

Making sure the inner queries are generated with aliases for each columns, which is something we already have the machinery for, should be enough to address the issue.

We just have to make sure the queries are compiled by passing with_col_aliases=True to their compiler's as_sql, something like

django/db/models/sql/compiler.py

@@ -563 +563 @@
                             *self.query.annotation_select,
                         )
                     )
-                part_sql, part_args = compiler.as_sql()
+                part_sql, part_args = compiler.as_sql(with_col_aliases=True)
                 if compiler.query.combinator:
                     # Wrap in a subquery if wrapping in parentheses isn't
                     # supported

[David Sanders]

I was going to take a look when I get bored with $job but it sounds like you already have it figured out xD

If you're too busy I can look though…

[Simon Charette]

David, I'd appreciate if you could have a look.

I'm pretty convinced the solution lies in using with_col_aliases=True and as long as the ordering clause is adjusted to refer to the column alias of the left-most query (the one combining the others) then it should work.

From some initial testing it might require a bit of adjustments in SQLCompiler.get_order_by and as_sql when self.pre_sql_setup is called so with_col_aliases=with_col_aliases or bool(self.query.combinator).

[David Sanders]

👍

[David Sanders]

Just FYI re:

django.db.utils.DatabaseError: ORDER BY not allowed in subqueries of compound statements.

SQLite doesn't support limits & ordering in compound statments: ​https://www.sqlite.org/lang_select.html#compound_select_statements

The error we're seeing here is a Django feature flag checking the presence of an order by. The Author model in the test has a default ordering which is causing this.

[Simon Charette]

I figured the origin of the 1st ORDER BY term does not match any column in the result set error; it's another regression introduced by c58a8acd413ccc992dd30afd98ed900897e1f719 but solely on SQLite 3.28 and 3.29 which should be fixed by ​this PR.

The select_related issue remains though. Any help required David?

[David Sanders]

Simon, correct the 1st ORDER BY term does not match any column in the result set on SQLite also happens because of the ambiguous reference in ORDER BY id from first() when using select_related().

I'm currently trying to determine a better way to determine an order by match rather than relying on aliases like is currently being done: ​https://github.com/django/django/blob/main/django/db/models/sql/compiler.py#L464-L467

(because if selects are always aliases then this is no longer something that can be relied on)

[Mariusz Felisiak]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In 70499b2:

Fixed #34123 -- Fixed combinator order by alias when using select_related().

Regression in c58a8acd413ccc992dd30afd98ed900897e1f719.

Thanks to Shai Berger for the report and tests.

Co-Authored-By: David Sanders <shang.xiao.sanders@…>