[Django] #36065: order_by("pk") not equivalent to order

Django

unread,

Jan 4, 2025, 1:06:20 PM1/4/25

to django-...@googlegroups.com

#36065: order_by("pk") not equivalent to order_by(F("pk")) for CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: dev | Severity: Release
| blocker
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
For CompositePrimaryKey, `order_by("pk")` and `order_by(F("pk"))` produce
different results.

For `"pk"`, direction is distributed to all cols `(col1 ASC, col2 ASC)`,
but for `F("pk")`, it is not: `(col1, col2 ASC)`.

Failing test:
{{{#!diff
diff --git a/tests/composite_pk/test_filter.py
b/tests/composite_pk/test_filter.py
index 7e361c5925..81bbfc65be 100644
--- a/tests/composite_pk/test_filter.py
+++ b/tests/composite_pk/test_filter.py
@@ -1,3 +1,4 @@
+from django.db.models import F
from django.test import TestCase

from .models import Comment, Tenant, User
@@ -78,6 +79,12 @@ class CompositePKFilterTests(TestCase):
),
)

+ def test_order_by_comments_by_pk_asc_f(self):
+ self.assertSequenceEqual(
+ Comment.objects.order_by("pk"),
+ Comment.objects.order_by(F("pk")),
+ )
+
def test_filter_comments_by_pk_gt(self):
c11, c12, c13, c24, c15 = (
self.comment_1,
}}}
{{{
======================================================================
FAIL: test_order_by_comments_by_pk_asc_f
(composite_pk.test_filter.CompositePKFilterTests.test_order_by_comments_by_pk_asc_f)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/user/django/tests/composite_pk/test_filter.py", line 83, in
test_order_by_comments_by_pk_asc_f
self.assertSequenceEqual(
AssertionError: Sequences differ: <Quer[123 chars] Comment object ((1,
5))>, <Comment: Comment object ((2, 4))>]> != <Quer[123 chars] Comment
object ((1, 5))>, <Comment: Comment object ((2, 4))>]>

<QuerySet [<Comment: Comment object ((1, 1))>, <Comment: Comment object
((1, 2))>, <Comment: Comment object ((1, 3))>, <Comment: Comment object
((1, 5))>, <Comment: Comment object ((2, 4))>]>

----------------------------------------------------------------------
(0.000)
SELECT "composite_pk_comment"."tenant_id",
"composite_pk_comment"."comment_id",
"composite_pk_comment"."user_id",
"composite_pk_comment"."text"
FROM "composite_pk_comment"
ORDER BY "composite_pk_comment"."tenant_id" ASC,
"composite_pk_comment"."comment_id" ASC;

args=();

ALIAS=DEFAULT (0.000)
SELECT "composite_pk_comment"."tenant_id",
"composite_pk_comment"."comment_id",
"composite_pk_comment"."user_id",
"composite_pk_comment"."text"
FROM "composite_pk_comment"
ORDER BY "composite_pk_comment"."tenant_id",
"composite_pk_comment"."comment_id" ASC;

args=();

ALIAS=DEFAULT
----------------------------------------------------------------------
Ran 103 tests in 0.107s

FAILED (failures=1)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36065>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,

Jan 6, 2025, 2:51:06 AM1/6/25

to django-...@googlegroups.com

#36065: order_by("pk") not equivalent to order_by(F("pk")) for CompositePrimaryKey
-------------------------------------+-------------------------------------

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* cc: Csirmaz Bendegúz (added)
* stage: Unreviewed => Accepted

Comment:

Thank you!
--
Ticket URL: <https://code.djangoproject.com/ticket/36065#comment:1>

Django

unread,

Jan 6, 2025, 11:02:12 AM1/6/25

to django-...@googlegroups.com

#36065: order_by("pk") not equivalent to order_by(F("pk")) for CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | 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):

The changes made to `SQLCompiler.find_ordering_name`
[https://github.com/django/django/commit/978aae4334fa71ba78a3e94408f0f3aebde8d07c
#diff-
f58de2deaccecd2d53199c5ca29e3e1050ec2adb80fb057cdfc0b4e6accdf14fR1119 to
accommodate for] `order_by(*: str)` references to a composite primary key
failed to account for the fact that resolvable and direct `OrderBy`
instance can be passed to `QuerySet.order_by`.

The solution likely lies in adjusting `find_ordering_name` to create a
`ColPairs` when there are multiple targets (instead of calling
`composite.unnest`) and adjusting `OrderBy.as_sql` to properly deal with
`ColPairs`. I guess we should test for `F("pk").asc(nulls_first=True)` and
`F("pk").asc(nulls_last=True)` (even if primary keys cannot contain nulls)
while we're at it to ensure the proper `NULLS LAST` emulation is working
correction.

{{{#!diff
diff --git a/django/db/models/expressions.py
b/django/db/models/expressions.py
index 667e9f93c6..2e1828ff9b 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -1850,6 +1850,16 @@ def get_source_expressions(self):
return [self.expression]

def as_sql(self, compiler, connection, template=None,
**extra_context):
+ if isinstance(self.expression, ColPairs):
+ sql_parts = []
+ params = []
+ for col in self.expression.get_cols():
+ copy = self.copy()
+ copy.set_source_expressions([col])
+ sql, col_params = compiler.compile(copy)
+ sql_parts.append(sql)
+ params.extend(col_params)
+ return ", ".join(sql_parts), params
template = template or self.template
if connection.features.supports_order_by_nulls_modifier:
if self.nulls_last:
diff --git a/django/db/models/sql/compiler.py
b/django/db/models/sql/compiler.py
index 5bb491d823..251cc08e51 100644
--- a/django/db/models/sql/compiler.py
+++ b/django/db/models/sql/compiler.py
@@ -1117,10 +1117,9 @@ def find_ordering_name(
)
return results
targets, alias, _ = self.query.trim_joins(targets, joins, path)
- target_fields = composite.unnest(targets)
return [
(OrderBy(transform_function(t, alias),
descending=descending), False)
- for t in target_fields
+ for t in targets
]

def _setup_joins(self, pieces, opts, alias):
diff --git a/tests/composite_pk/test_filter.py
b/tests/composite_pk/test_filter.py
index 7e361c5925..be0e80a518 100644

--- a/tests/composite_pk/test_filter.py
+++ b/tests/composite_pk/test_filter.py
@@ -1,3 +1,4 @@
+from django.db.models import F
from django.test import TestCase

from .models import Comment, Tenant, User

@@ -78,6 +79,12 @@ def test_order_comments_by_pk_desc(self):
),
)

+ def test_order_comments_by_pk_asc_f(self):
+ self.assertQuerySetEqual(

+ Comment.objects.order_by("pk"),
+ Comment.objects.order_by(F("pk")),
+ )
+
def test_filter_comments_by_pk_gt(self):
c11, c12, c13, c24, c15 = (
self.comment_1
}}}

Note that we'll need to mark `OrderBy.allows_composite_expressions = True`
per the work on #36042 depending on the order in which the changes are
merged.
--
Ticket URL: <https://code.djangoproject.com/ticket/36065#comment:2>

Django

unread,

Jan 6, 2025, 11:37:39 PM1/6/25

to django-...@googlegroups.com

#36065: order_by("pk") not equivalent to order_by(F("pk")) for CompositePrimaryKey
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: Simon
| Charette
Type: Bug | Status: assigned

Changes (by Simon Charette):

* owner: (none) => Simon Charette
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36065#comment:3>

Django

unread,

Jan 7, 2025, 12:11:15 AM1/7/25

to django-...@googlegroups.com

#36065: order_by("pk") not equivalent to order_by(F("pk")) for CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36065#comment:4>

Django

unread,

Jan 7, 2025, 10:40:24 AM1/7/25

to django-...@googlegroups.com

#36065: order_by("pk") not equivalent to order_by(F("pk")) for CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36065#comment:5>

Django

unread,

Jan 8, 2025, 4:20:59 AM1/8/25

to django-...@googlegroups.com

#36065: order_by("pk") not equivalent to order_by(F("pk")) for CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"7617d5be94a6e348d5ddf4644985b24235822034" 7617d5be]:
{{{#!CommitTicketReference repository=""
revision="7617d5be94a6e348d5ddf4644985b24235822034"
Refs #36065 -- Extracted composite primary key order by tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36065#comment:6>

Django

unread,

Jan 8, 2025, 4:20:59 AM1/8/25

to django-...@googlegroups.com

#36065: order_by("pk") not equivalent to order_by(F("pk")) for CompositePrimaryKey
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Simon
| Charette

Type: Bug | Status: closed

Component: Database layer | Version: dev
(models, ORM) |

Severity: Release blocker | Resolution: fixed

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"42e8f264ce55710056b0033682ec6fd662a25b29" 42e8f264]:
{{{#!CommitTicketReference repository=""
revision="42e8f264ce55710056b0033682ec6fd662a25b29"
Fixed #36065 -- Fixed ordering by expression referencing composite primary
key.

Thanks Jacob Walls for the report and test and Csirmaz Bendegúz for the
review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36065#comment:7>

Reply all

Reply to author

Forward

[Django] #36065: order_by("pk") not equivalent to order_by(F("pk")) for CompositePrimaryKey

Django

Django

Django

Django

Django

Django

Django

Django