Opened 4 years ago
Closed 4 years ago
#32944 closed Cleanup/optimization (fixed)
Simplify where clause creation where the WhereNode is immediately created and then added to and then returned.
| Reported by: | Keryn Knight | Owned by: | Keryn Knight |
|---|---|---|---|
| Component: | Database layer (models, ORM) | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | yes |
| Easy pickings: | no | UI/UX: | no |
Description
There are currently 2 places in Django which essentially do:
where = WhereNode() where.add(condition, AND)
and then return the where.
There are a bunch of places where a where class is added to and then conditionally added to either immediately or far later in other complex branches, (eg: is it a reffed expression, etc). Those are not for consideration IMHO because the changes would probably add more complexity.
But for those 2 places, django.db.models.sql.query.Query.build_filter and django.contrib.contenttypes.fields.GenericRelation.get_extra_restriction where the code is respectively:
clause = self.where_class() clause.add(condition, AND) return clause, []
and
cond = where_class()
lookup = field.get_lookup('exact')(field.get_col(remote_alias), contenttype_pk)
cond.add(lookup, 'AND')
return cond
both can be simplified to respectively:
return self.where_class([condition], connector=AND), []
and
return where_class([lookup], connector=AND)
which end up as the same result, that is:
x = WhereNode([('a', 1)], connector='AND')
y = WhereNode()
y.add(('a', 1), 'AND')
assert x == y
but a bit faster. Specifically the proposed form is roughly 550-600ns per op vs. the unnecessary add which is 650-700ns:
In [17]: %%timeit
...: x = WhereNode([('a', 1)], connector='AND')
597 ns ± 12.5 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
In [18]: %%timeit
...: x = WhereNode()
...: x.add(('a', 1), 'AND')
780 ns ± 8.73 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
Given build_filter prevalence (it's called what, at least once per argument to filter() or exclude() ... maybe more on complex queries?) it's maybe worth having.
I'll put in a PR shortly to run the bits of the test suite I can't, and boil the ocean on the off-chance that this gets accepted. Naturally if anything fails, we can just move it to invalid without further consideration :)
Change History (3)
comment:1 by , 4 years ago
| Owner: | changed from to |
|---|
comment:2 by , 4 years ago
| Has patch: | set |
|---|---|
| Patch needs improvement: | set |
| Triage Stage: | Unreviewed → Accepted |
PR