[Django] #34450: Lookup expressions across foreign keys introduce extra joins
Django
-------------------------------------+-------------------------------------
Reporter: Roman | Owner: nobody
Odaisky |
Type: Bug | Status: new
Component: Database | Version: 4.1
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Assuming Child has a ForeignKey to Parent,
{{{
Parent.objects.filter(child__x="x", child__y="y")
}}}
results in
{{{
SELECT ...
FROM "app_parent"
INNER JOIN "app_child" ON ("app_parent"."id" = "app_child"."parent_id")
WHERE ("app_child"."x" = ('x') AND "app_child"."y" = ('y'))
}}}
which makes perfect sense.
However,
{{{
Parent.objects.filter(Exact(F("child__x"), "x"), child__y="y")
}}}
unexpectedly produces
{{{
SELECT ...
FROM "app_parent"
LEFT OUTER JOIN "app_child" ON ("app_parent"."id" =
"app_child"."parent_id")
INNER JOIN "app_child" T3 ON ("app_parent"."id" = T3."parent_id")
WHERE ("app_child"."x" = ('x') AND T3."y" = ('y'))
}}}
basically converting an AND lookup into an OR, which canβt be correct.
Same error here:
{{{
Parent.objects.filter(Exact(F("child__x"), "x") & Q(child__y="y"))
}}}
But these work correctly:
{{{
Parent.objects.filter(Q(child__y="y") & Exact(F("child__x"), "x"))
Parent.objects.filter(Exact(F("child__x"), "x") & Exact(F("child__y"),
"y"))
}}}
It would seem that all of the above should produce the same SQL as the
first invocation instead of introducing extra joins that quietly turn ANDs
into ORs, and most definitely there should be no dependence on the order
of the operands.
--
Ticket URL: <https://code.djangoproject.com/ticket/34450>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Hi, thanks for the report π
Confirmed on latest main that a left outer join is produced.
--
Ticket URL: <https://code.djangoproject.com/ticket/34450#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Odaisky | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | 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 David Sanders):
Some testing:
Correct joining:
{{{
query = Query(Parent)
query.add_q(Q(child__x__exact= 'x'))
query.add_q(Q(Exact(F("child__x"), 'x')))
print(query)
SELECT "ticket_34450_parent"."id" FROM "ticket_34450_parent" INNER JOIN
"ticket_34450_child" ON ("ticket_34450_parent"."id" =
"ticket_34450_child"."parent_id") WHERE ("ticket_34450_child"."x" = x AND
"ticket_34450_child"."x" = (x))
}}}
On the other hand with the order reversed:
{{{
query = Query(Parent)
query.add_q(Q(Exact(F("child__x"), 'x')))
query.add_q(Q(child__x__exact= 'x'))
print(query)
SELECT "ticket_34450_parent"."id" FROM "ticket_34450_parent" LEFT OUTER
JOIN "ticket_34450_child" ON ("ticket_34450_parent"."id" =
"ticket_34450_child"."parent_id") INNER JOIN "ticket_34450_child" T3 ON
("ticket_34450_parent"."id" = T3."parent_id") WHERE
("ticket_34450_child"."x" = (x) AND T3."x" = x)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34450#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Odaisky | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | 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):
`QuerySet.filter` results in a single `Query.add_q` so I'm not sure that
comparing the effect of two distinct `add_q` call should be the area of
focus here as the latter is more alike to `filter()` chaining calls (e.g.
`QuerySet.filter().filter()`) which has different semantic when dealing
with multi-valued relationships.
--
Ticket URL: <https://code.djangoproject.com/ticket/34450#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Odaisky | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | 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):
Haven't run the full test suite yet but it seems that intuitively passing
`reuse` so the JOINs are reused does the trick for this particular issue
at least
{{{#!python
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index 3cb8bdbc8a..1ed7d7cb6e 100644
--- a/django/db/models/sql/query.py
+++ b/django/db/models/sql/query.py
@@ -1373,7 +1373,7 @@ def build_filter(
if not getattr(filter_expr, "conditional", False):
raise TypeError("Cannot filter against a non-conditional
expression.")
condition = filter_expr.resolve_expression(
- self, allow_joins=allow_joins, summarize=summarize
+ self, allow_joins=allow_joins, reuse=can_reuse,
summarize=summarize,
)
if not isinstance(condition, Lookup):
condition = self.build_lookup(["exact"], condition, True)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34450#comment:4>
Django
-------------------------------------+-------------------------------------
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Simon Charette
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34450#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Odaisky | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | 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):
Roman, can you confirm [https://github.com/django/django/pull/16710 this
PR] addresses your issue?
--
Ticket URL: <https://code.djangoproject.com/ticket/34450#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Odaisky | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34450#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Odaisky | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Sanders):
@Simon
> so I'm not sure that comparing the effect of two distinct add_q call
should be the area of focus
Sure, I'm not saying that was the area of focus, just that it was some
interesting behaviour I saw that Query will try to demote if possible
based on existing inner joins (which appears to be done in `add_q()`)
--
Ticket URL: <https://code.djangoproject.com/ticket/34450#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Odaisky | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Sanders):
Oh and it also raises the question of whether
{{{
Parent.objects.filter(Exact(F("child__x"), "x")
}}}
should be demoted (produce an inner join) because at the moment
`build_filter()` doesn't attempt to find any "needed inners" thus
`_add_q()` doesn't have anything to try to demote to.
π
--
Ticket URL: <https://code.djangoproject.com/ticket/34450#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Odaisky | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: David Sanders (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34450#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Odaisky | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Sanders):
Just FYI Simon's patch resolves the reported issue:
{{{
> print(Parent.objects.filter(Exact(F("child__x"), "x"),
child__y="y").query)
SELECT "ticket_34450_parent"."id" FROM "ticket_34450_parent" INNER JOIN
"ticket_34450_child" ON ("ticket_34450_parent"."id" =
"ticket_34450_child"."parent_id") WHERE ("ticket_34450_child"."x" = (x)
AND "ticket_34450_child"."y" = y)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34450#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Roman Odaisky | Owner: Simon
| Charette
Component: Database layer | Version: 4.1
(models, ORM) |
Keywords: | Triage Stage: Accepted
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:"0e1aae7a5f51408b73c5a29e18bd1803dd030930" 0e1aae7a]:
{{{
#!CommitTicketReference repository=""
revision="0e1aae7a5f51408b73c5a29e18bd1803dd030930"
Fixed #34450 -- Fixed multi-valued JOIN reuse when filtering by
expressions.
Thanks Roman Odaisky for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34450#comment:12>