[Django] #28211: Inefficient query produced when using OR'ed Q objects
Django
-------------------------------------+-------------------------------------
Reporter: Tom | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: 1.10
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 |
-------------------------------------+-------------------------------------
When using OR'ed Q objects in a `.filter()` call Django could be more
intelligent about the query it produces.
For example, the following two queries both produce `LEFT OUTER JOIN`:
`SomeModel.objects.filter(Q(related_objects__field=1) |
Q(related_objects__other_field=1)`
`SomeModel.objects.filter(Q(related_objects__field=1) | Q())`
In the case of the second query it could be reduced to a `INNER JOIN` as
the `OR` is redundant and it is functionally the same as a
`.filter(related_objects__field=1)`.
It is also quite a common pattern to do:
```
filters = Q()
if condition:
filters |= Q(x=1)
if other_condition:
filters |= Q(y=2)
```
And with the current implementation it will always produce a query that
assumes `filters` is a valid `OR`. Django should/could be more intelligent
and detect if there is only one `OR` condition, and reduce it.
--
Ticket URL: <https://code.djangoproject.com/ticket/28211>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Tom | Owner: nobody
Type: | Status: new
Cleanup/optimization |
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> When using OR'ed Q objects in a `.filter()` call Django could be more
> intelligent about the query it produces.
>
> For example, the following two queries both produce `LEFT OUTER JOIN`:
>
> `SomeModel.objects.filter(Q(related_objects__field=1) |
> Q(related_objects__other_field=1)`
>
> `SomeModel.objects.filter(Q(related_objects__field=1) | Q())`
>
> In the case of the second query it could be reduced to a `INNER JOIN` as
> the `OR` is redundant and it is functionally the same as a
> `.filter(related_objects__field=1)`.
>
> It is also quite a common pattern to do:
>
> ```
> filters = Q()
> if condition:
> filters |= Q(x=1)
> if other_condition:
> filters |= Q(y=2)
> ```
>
> And with the current implementation it will always produce a query that
> assumes `filters` is a valid `OR`. Django should/could be more
> intelligent and detect if there is only one `OR` condition, and reduce
> it.
New description:
When using OR'ed Q objects in a `.filter()` call Django could be more
intelligent about the query it produces.
For example, the following two queries both produce `LEFT OUTER JOIN`:
`SomeModel.objects.filter(Q(related_objects__field=1) |
Q(related_objects__other_field=1)`
`SomeModel.objects.filter(Q(related_objects__field=1) | Q())`
In the case of the second query it could be reduced to a `INNER JOIN` as
the `OR` is redundant and it is functionally the same as a
`.filter(related_objects__field=1)`.
It is also quite a common pattern to do:
{{{
filters = Q()
if condition:
filters |= Q(x=1)
if other_condition:
filters |= Q(y=2)
}}}
And with the current implementation it will always produce a query that
assumes `filters` is a valid `OR`. Django should/could be more intelligent
and detect if there is only one `OR` condition, and reduce it.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28211#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Tom | Owner: nobody
Type: | Status: new
Cleanup/optimization |
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Tentatively accepting, though I don't know if this is feasible or correct.
If a patch is provided, hopefully the test suite will validate its
correctness.
--
Ticket URL: <https://code.djangoproject.com/ticket/28211#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Tom | Owner: nobody
Type: | Status: new
Cleanup/optimization |
(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
Comment:
I added a patch here: https://github.com/django/django/pull/8517
I took the easy route and fixed what I think is an issue in the `Q`
object. Combining empty `Q` objects should be a no-op. This effectively
fixes this ticket without many changes.
--
Ticket URL: <https://code.djangoproject.com/ticket/28211#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Tom | Owner: nobody
Type: | Status: new
Cleanup/optimization |
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 1.10 => master
* stage: Accepted => Ready for checkin
Comment:
I think the provided patch solves this in an efficient way.
We could add folding logic in the query compiler as well but given it will
be very hard to generate combined `Q` with a falsey sides after with this
patch (one would have to call `Q.add` manually) I don't think it's
required.
--
Ticket URL: <https://code.djangoproject.com/ticket/28211#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Tom | Owner: nobody
Type: | Status: new
Cleanup/optimization |
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"1c3a6cec2ae35c4326971e62acbc1891506e7e72" 1c3a6ce]:
{{{
#!CommitTicketReference repository=""
revision="1c3a6cec2ae35c4326971e62acbc1891506e7e72"
Refs #28211 -- Added a test for ANDing empty Q()'s.
This test passes to due some logic in Node.add().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28211#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Tom | Owner: nobody
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"bb0b6e526340e638522e093765e534df4e4393d2" bb0b6e52]:
{{{
#!CommitTicketReference repository=""
revision="bb0b6e526340e638522e093765e534df4e4393d2"
Fixed #28211 -- Prevented ORing an empty Q() from reducing query join
efficiency.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28211#comment:6>