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 Keryn Knight, 3 years ago

Owner: changed from nobody to Keryn Knight

comment:2 by Mariusz Felisiak, 3 years ago

Has patch: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

comment:3 by GitHub <noreply@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 6a970a8:

Fixed #32944 -- Avoided unnecessary WhereNode.add() calls.

Co-authored-by: Mariusz Felisiak <felisiak.mariusz@…>

Note: See TracTickets for help on using tickets.
Back to Top