Opened 20 months ago
Last modified 3 weeks ago
#34533 assigned Bug
OuterRef not resolved as part of ORDER BY clause
Reported by: | REGNIER Guillaume | Owned by: | Ayush Khatri |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 3.2 |
Severity: | Normal | Keywords: | OuterRef, OrderBy |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
I feel like OuterRef are not resolved properly where used for ordering.
Here is a simple example showcasing the issue:
MyModel.objects.annotate(foo=Subquery(MyModel.objects.order_by("pk" - OuterRef("pk")).values("pk")[:1]))
The above fails with :
ValueError: This queryset contains a reference to an outer query and may only be used in a subquery.
Because the as_sql method of ResolvedOuterRef is called.
I think this a bug because it did not raise any notice regarding OuterRef and order_by not being compatible.
Change History (21)
comment:1 by , 20 months ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 19 months ago
I've been working in and out on this for the last three weeks. I'm sorry I didn't claim the ticket...
Here is a branch where the issue is solved: https://github.com/Alombaros/django/tree/ticket_34533_3_2
I'm unsure on how to proceed.
Previously, in the following query :
MyModel.objects.annotate(foo=Subquery(MyOtherModel.objects.order_by(F("pk")).values("pk")[:1]))
The F("pk")
was resolved as the pk
of the outer query (MyModel
in this case) which does not seems right to me.
This was due to the order by expressions not being resolved until the last minute.
I force the resolution when annotating a SubQuery so that the OuterRef can do its job but this made so F
objects are resolved as field of the inner query.
To me, this seems to be closer to the intended behavior because its what append when calling .filter
or .annotate
but if anyone used the prior behavior, thing will break
comment:4 by , 19 months ago
Owner: | removed |
---|---|
Status: | assigned → new |
no worry! i think it's better tyou bring this ticket. i deassign this ticket.
comment:5 by , 19 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:6 by , 15 months ago
Owner: | changed from | to
---|
comment:9 by , 11 months ago
Owner: | changed from | to
---|
I will try to improve what can be done from previous PR.
follow-up: 11 comment:10 by , 11 months ago
The hard part is here is likely due to two factor
OuterRef
resolving is quite finicky and depends on a specific chain of resolving during query composition (subquery is resolved and then the outer query is)order_by
members are unfortunately not resolved at query composition time (whenQuerySet.order_by
is called) but at query compilation time.
The reason for 2. is that QuerySet.order_by
calls are not additive but destructive and that the query composition logic doesn't support a generic way of eliding joins that we previously created for some operations.
For example, say that you have a query that does qs = Book.objects.order_by("author__name")
. If the order by clause is immediately resolved then a JOIN
to author
must be created. If the same query has then its ordering changed to qs.order_by("title")
then we'd want to clear the join to author
as it's no longer necessary.
In order to avoid implementing the logic to unreference relations the order_by
method deferred the resolving to the very end of query compilation which is a different path than the normal resolving taking place in additive methods such as annotate
and filter
.
I suspect this ticket will be hard to solve without tackling the large problem of compile time resolving of order_by
.
comment:11 by , 11 months ago
Replying to Simon Charette:
The hard part is here is likely due to two factor
OuterRef
resolving is quite finicky and depends on a specific chain of resolving during query composition (subquery is resolved and then the outer query is)order_by
members are unfortunately not resolved at query composition time (whenQuerySet.order_by
is called) but at query compilation time.The reason for 2. is that
QuerySet.order_by
calls are not additive but destructive and that the query composition logic doesn't support a generic way of eliding joins that we previously created for some operations.
For example, say that you have a query that does
qs = Book.objects.order_by("author__name")
. If the order by clause is immediately resolved then aJOIN
toauthor
must be created. If the same query has then its ordering changed toqs.order_by("title")
then we'd want to clear the join toauthor
as it's no longer necessary.
In order to avoid implementing the logic to unreference relations the
order_by
method deferred the resolving to the very end of query compilation which is a different path than the normal resolving taking place in additive methods such asannotate
andfilter
.
I suspect this ticket will be hard to solve without tackling the large problem of compile time resolving of
order_by
.
Thank you for the pointer! Since this ticket is complicated, I'll leave it to someone who is interested in solving this problem.
comment:12 by , 11 months ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:14 by , 10 months ago
You're welcome to try with the disclaimer that you understand what Simon has said above about it being quite hard and it will take an in-depth understanding of how expressions are resolved & compiled :)
comment:15 by , 10 months ago
Thank you for answer.
I have a question.
Has this error been resolved in version 4.0 or higher?
comment:16 by , 10 months ago
@HeejunShin, you can try running the example from the ticket Description - see if that works with the main branch. I don't think this issue has been fixed, though.
comment:17 by , 5 months ago
I am able to reproduce this issue with a simple unit test still against the main branch.
-
tests/queries/tests.py
diff --git a/tests/queries/tests.py b/tests/queries/tests.py index ec88fa558d..a9dd943ee8 100644
a b class SubqueryTests(TestCase): 2377 2377 NamedCategory.objects.create(id=3, name="third") 2378 2378 NamedCategory.objects.create(id=4, name="fourth") 2379 2379 2380 def test_outer_ref_order_by(self): 2381 values = NamedCategory.objects.annotate( 2382 foo=NamedCategory.objects.order_by(OuterRef("pk").desc()).values("name") 2383 ).first() 2384 2385 self.assertEqual(["fourth", "third", "second", "first"], values.foo) 2386 2380 2387 def test_ordered_subselect(self): 2381 2388 "Subselects honor any manual ordering" 2382 2389 query = DumbCategory.objects.filter(
comment:18 by , 4 weeks ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:20 by , 3 weeks ago
Patch needs improvement: | unset |
---|---|
Summary: | OuterRef not resolved as part oh ORDER BY clause → OuterRef not resolved as part of ORDER BY clause |
Thanks for linking the PR, Ayush, and when assuming a ticket you can unset Patch Needs Improvement to ensure you get an initial review.
comment:21 by , 3 weeks ago
Patch needs improvement: | set |
---|
Thanks 👍
Just FYI this will work (note you must wrap 'pk' with an F):