Re: [Django] #34123: Ambiguous aliases in ordering on combined queries with select_related().
Django
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: select_related | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by David Sanders):
* cc: David Sanders (added)
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34123#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: select_related | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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
{{{#!diff
diff --git a/django/db/models/sql/compiler.py
b/django/db/models/sql/compiler.py
index 3097500be4..8b727f42ce 100644
--- a/django/db/models/sql/compiler.py
+++ b/django/db/models/sql/compiler.py
@@ -563,7 +563,7 @@ def get_combinator_sql(self, combinator, all):
*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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34123#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: select_related | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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…
--
Ticket URL: <https://code.djangoproject.com/ticket/34123#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: select_related | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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` as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/34123#comment:5>
Django
-------------------------------------+-------------------------------------
| Sanders
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: select_related | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => David Sanders
* status: new => assigned
Comment:
👍
--
Ticket URL: <https://code.djangoproject.com/ticket/34123#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: David
| Sanders
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: select_related | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34123#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: David
| Sanders
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: select_related | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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 3.28 and 3.29 which
should be fixed by [https://github.com/django/django/pull/16244 this PR].
The `select_related` issue remains though. Any help required David?
--
Ticket URL: <https://code.djangoproject.com/ticket/34123#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: David
| Sanders
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: select_related | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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)
--
Ticket URL: <https://code.djangoproject.com/ticket/34123#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: David
| Sanders
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/16296 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34123#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Shai Berger | Owner: David
| Sanders
Component: Database layer | Version: dev
(models, ORM) |
Keywords: select_related | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"70499b25c708557fb9ee2264686cd172f4b2354e" 70499b2]:
{{{
#!CommitTicketReference repository=""
revision="70499b25c708557fb9ee2264686cd172f4b2354e"
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.xia...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34123#comment:11>