Opened 3 months ago
Closed 2 months ago
#35788 closed Cleanup/optimization (wontfix)
Order By using column number with Annotated fields
Reported by: | Adrian Garcia | Owned by: | |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 5.1 |
Severity: | Normal | Keywords: | order_by, annotate, column number, |
Cc: | Adrian Garcia, Simon Charette | Triage Stage: | Unreviewed |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
As the title states, .order_by()
is using a deprecated method of selecting which column to order by. While most modern DBs still allow this, the use of constants is discouraged.
>>> from django.db import models >>> class Test(models.Model): >>> name = models.CharField() >>> class Meta: >>> app_label = "test" >>> >>> # Use of a constant when referencing an annotation >>> Test.objects.all().annotate(test_annotation = models.F("name")).order_by("test_annotation").query SELECT "test_test"."id", "test_test"."name", "test_test"."name" AS "test_annotation" FROM "test_test" ORDER BY 3 ASC >>> >>> # Use of column name when not referencing annotations: >>> Test.objects.all().annotate(test_annotation = models.F("name")).order_by("name").query SELECT "test_test"."id", "test_test"."name", "test_test"."name" AS "test_annotation" FROM "test_test" ORDER BY "test_test"."name" ASC
I'd be happy to draft a PR for this if this is deemed something worth addressing.
Change History (5)
comment:1 by , 3 months ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
comment:2 by , 3 months ago
The MySQL documentation references its removal from the standard. Additionally, this article specifically references that the use of constants was defined in the ANSI SQL-92 standard, and subsequently removed in ANSI SQL-99, but goes on to say that most RDBMS vendors still support this practice.
All DBs that Django currently supports (and many that are supported via third party libraries) appear to honor the deprecated column numbers, and given how long ago this change was made it's likely they will continue to support this. It's mostly the inconsistency of order_by
using an integer constant _only_ with annotated fields that bothers me, rather than any risk of the feature suddenly breaking, which is why I offered to make the change if that's acceptable.
Since these older versions of the SQL spec can be found online for free, I was able to find the actual definitions.
From page pages 371 and 372 of ANSI SQL-92:
<order by clause> ::= ORDER BY <sort specification list> <sort specification list> ::= <sort specification> [ { <comma> <sort specification> }... ] <sort specification> ::= <sort key> [ <collate clause > ] [ <ordering specification> ] <sort key> ::= <column name> | <unsigned integer> <ordering specification> ::= ASC | DESC ... 10)If ORDER BY is specified, then each <sort specification> in the <order by clause> shall identify a column of T. Case: a) If a <sort specification> contains a <column name>, then T shall contain exactly one column with that <column name> and the <sort specification> identifies that column. b) If a <sort specification> contains an <unsigned integer>, then the <unsigned integer> shall be greater than 0 and not greater than the degree of T. The <sort specification> identifies the column of T with the ordinal position specified by the <unsigned integer>.
From page 651 of ANSI SQL-99
<order by clause> ::= ORDER BY <sort specification list> <sort specification list> ::= <sort specification> [ { <comma> <sort specification> }... ] <sort specification> ::= <sort key> [ <collate clause> ] [ <ordering specification> ] <sort key> ::= <value expression> <ordering specification> ::= ASC | DESC ... NOTE 287 – A previous version of ISO/IEC 9075 allows <sort specification> to be a <signed in- teger> to denote a column reference of a column of T. That facility no longer exists. See Annex E, ‘‘Incompatibilities with ISO/IEC 9075:1992 and ISO/IEC 9075-4:1996’’.
comment:3 by , 2 months ago
Resolution: | needsinfo |
---|---|
Status: | closed → new |
comment:4 by , 2 months ago
Cc: | added |
---|
I'm curious about whether there is any bug or breakage that using index based ORDER BY
or GROUP BY
causes or if it's solely an ideological request to adhere to the SQL spec (which the queries the ORM generates often diverges from).
I'm asking because there are two legitimate reasons I can think of why the ORM does that for GROUP BY
and ORDER BY
clauses
- To avoid ambiguity when referencing columns with colliding aliases; see #34346 and #34176 which introduced the changes
- To support prepared statements and server-side parameters binding without requiring a significant rewrite of how SQL is generated for
GROUP BY
andORDER BY
clause; see #35028 and #34255 which uncovered the problem and #20516 which discussed the addition of prepared statements.
comment:5 by , 2 months ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Given the input from Simon, I feel we can accept this ticket if we have encountered a bug or breakage due to using index based ORDER BY
.
Adrian, if you find something, feel free to comment and reopen the ticket
Hi Adrian, can you share some references as to why this shouldn't be used?
I can't find anything saying any reasons this should be a concern