Opened 3 years ago
Closed 3 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 , 3 years ago
Owner: | changed from | to
---|
comment:2 by , 3 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
PR