[Django] #32946: Prefer non-kwargs construction of dynamically generated Q objects
Django
-------------------------------------+-------------------------------------
Reporter: Keryn | Owner: Keryn Knight
Knight |
Type: | Status: assigned
Cleanup/optimization |
Component: Database | Version: dev
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 |
-------------------------------------+-------------------------------------
There are a number of places where Q objects are created dynamically to do
things like construct a big OR expression, but they do so in the
historically available API way; because once upon a time there was no way
to pass the connector in the Q constructor, to make a OR node would've
required doing:
{{{
x = Q(a=1, b=2)
x.connector = Q.OR
}}}
which understandably was not the ideal from the general public API
standpoint, and so everyone re-uses the `|` pattern.
But that changed way back when, in
508b5debfb16843a8443ebac82c1fb91f15da687 for #11964, and now it ''can'' be
done as `Q(a=1, b=2, _connector=Q.OR)`
This is important because it's a substantially faster way of constructing
a single Q object which does the right thing, because it ignores the
complexity of cloning an instance and combining the parts into it. Here
follows demonstration data.
The smallest data point to consider is that sending in kwargs is
(unfortunately) slower because they're sorted (which I don't think is
correct, particularly, but that's a separate ticket tbh):
{{{
In [2]: %timeit Q(a=1, b=2, c=3, d=4)
1.85 µs ± 14.8 ns per loop (mean ± std. dev. of 7 runs, 1000000
loops each)
In [3]: %timeit Q(('a', 1), ('b', 2), ('c', 3), ('d', 4))
1.2 µs ± 10.4 ns per loop (mean ± std. dev. of 7 runs, 1000000
loops each)
In [4]: Q(a=1, b=2, c=3, d=4) == Q(('a', 1), ('b', 2), ('c', 3), ('d', 4))
True
}}}
Now, one of the common ways to build a dynamic Q is to use
`functools.reduce` and `operator.or_`, like the below, which is a
facsimile of the usage in Django and elsewhere:
{{{
In [1]: from functools import reduce
...: from operator import or_
...: from django.db.models.query_utils import Q
In [2]: values = (Q(a=1), Q(b=2), Q(c=3), Q(d=4), Q(e__in=[1,2,3,4]))
In [3]: %timeit reduce(or_, values)
12.3 µs ± 91 ns per loop (mean ± std. dev. of 7 runs, 100000 loops
each)
In [4]: %timeit Q(a=1, b=2, c=3, d=4, e__in=[1,2,3,4], _connector=Q.OR)
2.02 µs ± 23.5 ns per loop (mean ± std. dev. of 7 runs, 100000
loops each)
In [5]: %timeit Q(('a', 1), ('b', 2), ('c', 3), ('d', 4), ('e__in', [1, 2,
3, 4]), _connector=Q.OR)
1.4 µs ± 2.31 ns per loop (mean ± std. dev. of 7 runs, 1000000
loops each)
}}}
In the reduce given above, that's 5 Q objects created initially, and then
4 additional Q objects created, vs 1 for the latter. Which is basically
borne out by the timings, `1.4 * 9 == 12.6`.
Another usage which comes up is to do something like `Q(**{field.name:
xxx})` when `Q((field.name, xxx))` would suffice.
Or doing something like:
{{{
x = models.Q()
if y:
x |= models.Q(**{field: '!'})
if z:
x |= models.Q(**{field: '?'})
}}}
when it could be:
{{{
x = []
if y:
x.append((field, '!'))
if z:
x.append((field, '?'))
q = Q(x, _connector=Q.OR)
}}}
and so on.
I have a patch which targets most/all? of the constructions I think it
might make sense to change, and assuming the CI passes we can discuss from
there.
I also think that `Q(a=1, b=2, _connector=Q.OR)` and/or the `reduce(...)`
should be documented in the
https://docs.djangoproject.com/en/3.2/topics/db/queries/#complex-lookups-
with-q section. Dynamic Q construction is fairly common IME, doesn't
appear to be covered there, and ultimately points to Q test cases which
''also'' don't show any form of dynamic variants.
--
Ticket URL: <https://code.djangoproject.com/ticket/32946>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: | Knight
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(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:
Agreed.
> I also think that `Q(a=1, b=2, _connector=Q.OR)` and/or the
`reduce(...)` should be documented in the
https://docs.djangoproject.com/en/3.2/topics/db/queries/#complex-lookups-
with-q section. Dynamic Q construction is fairly common IME, doesn't
appear to be covered there, and ultimately points to Q test cases which
''also'' don't show any form of dynamic variants.
Using `Q(a=1, b=2, _connector=...)` internally is fine, but I don't think
we would like to encourage users to use it. I'd add an example with
`reduce()` which was mentioned in several tickets.
--
Ticket URL: <https://code.djangoproject.com/ticket/32946#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Knight
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
PR is https://github.com/django/django/pull/14699
As I write this, CI is still running, but I've set patch needs improvement
based on not having removed the imports for `functools`/`operator` ...
--
Ticket URL: <https://code.djangoproject.com/ticket/32946#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Knight
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(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
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32946#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Knight
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(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 Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"5b8ef8aa5b746d96d6558efd6bc119f347002703" 5b8ef8aa]:
{{{
#!CommitTicketReference repository=""
revision="5b8ef8aa5b746d96d6558efd6bc119f347002703"
Refs #32946 -- Changed Query.add_filter() to take two arguments.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32946#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Knight
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(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 Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"9662193aea2ee982bc8e553c62499aca5e606755" 9662193]:
{{{
#!CommitTicketReference repository=""
revision="9662193aea2ee982bc8e553c62499aca5e606755"
Refs #32946 -- Changed internal usage of dynamic Q() objects construction
to use non-kwargs initialization.
This prefers non-kwargs construction of dynamically generated Q()
objects to create a single Q() object instead of many and then
combining them, where possible.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32946#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Knight
Component: Database layer | Version: dev
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/32946#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Knight
Cleanup/optimization | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"9dff316be41847c505ebf397e4a52a0a71e0acc4" 9dff316b]:
{{{
#!CommitTicketReference repository=""
revision="9dff316be41847c505ebf397e4a52a0a71e0acc4"
Refs #32948, Refs #32946 -- Used Q.create() internally for dynamic Q()
objects.
Node.create() which has a compatible signature with Node.__init__()
takes in a single `children` argument rather than relying in unpacking
*args in Q.__init__() which calls Node.__init__().
In addition, we were often needing to unpack iterables into *args and
can instead pass a list direct to Node.create().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32946#comment:7>