Optimize q filter to use pk__in subqueries instead of set operations#7848
Optimize q filter to use pk__in subqueries instead of set operations#7848acheng-01 wants to merge 2 commits into
Conversation
The `q` complex filter composed its NOT/AND/OR operands with QuerySet
.difference()/.intersection()/.union(), which compile to SQL
EXCEPT/INTERSECT/UNION. For NOT in particular, the left operand is the
full unfiltered queryset, so evaluating an expression like
`repository_version=X AND NOT repository_version=Y` seq-scans the entire
content table and sorts/hashes every leg, spilling to disk on large
repositories.
Rewrite the three operand evaluators to compose with `pk__in` subqueries:
- _NotAction: qs.exclude(pk__in=expr.evaluate(qs).values("pk"))
- _AndAction: chained qs.filter(pk__in=...) semi-joins
- _OrAction: OR of Q(pk__in=...) subqueries
This collapses the plan to indexed anti-/semi-joins with no full-table
scan, no SetOp, and no disk-spilling sorts. Results are unchanged (each
operand still filters the same base queryset on a unique pk, so the
implicit de-duplication of the set operations is preserved); the
existing q filter functional tests pass without modification.
Benchmarked on a 200k-unit repository computing a 1000-unit diff
between two repository versions (steady-state, work_mem=4MB):
Version sizes Before (set-ops) After (pk__in)
--------------- ------------------ ----------------
199k units ~1400 ms ~680 ms
50k units ~450 ms ~150 ms
Planning time also drops (e.g. ~1.4 ms -> ~0.5 ms) as the large inlined
operand lists are replaced by parameterized subqueries.
Closes pulp#7831
Assisted by: Claude Opus 4.8
5d1d31a to
f6efa56
Compare
| class _OrAction: | ||
| def __init__(self, tokens): | ||
| self.exprs = tokens[0] | ||
| self.complexity = sum((expr.complexity for expr in self.exprs)) + 1 |
There was a problem hiding this comment.
FYI, the complexity is meant to prevent easy dos attacks by generating arbitrarily complex queries. I guess you can already get quite far with the current setting.
mdellweg
left a comment
There was a problem hiding this comment.
I would be willing to go this way.
It sounds like it can be a performance boost for all uses of q.
Do I understand correctly that your concern is manly how the performance is impacted by deeply nesting constructs?
| @@ -0,0 +1,3 @@ | |||
| Optimized the ``q`` complex filter to compose ``NOT``/``AND``/``OR`` operands using ``pk__in`` | |||
| subqueries instead of the ``EXCEPT``/``INTERSECT``/``UNION`` set operations, avoiding full-table | |||
| scans and disk-spilling sorts on large content sets. No newline at end of file | |||
There was a problem hiding this comment.
This is supposed to be markdown, not ReST.
Single backticks work better, as far as I recall.
Kind of. The benchmark copilot ran yesterday were done from more theoretical flattened SQL queries that content WHERE pk IN (SELECT pk FROM content WHERE pk IN (
SELECT pk FROM content WHERE pk IN (SELECT unnest(content_ids) FROM version)))Switching from set-ops to |
Builds on the prior pk__in subquery rewrite. That change removed the
set-operations, but still wrapped every operand of a NOT/AND/OR in its
own pk__in subquery, so a q expression compiled to one subquery per
operand and the SQL grew a nesting level for every AND/OR/NOT -- up to 8
stacked subqueries at the complexity cap. This is the "deep nesting"
cost the reviewer asked about.
Introduce a hybrid Q-composition. Each operand can now report an
`as_q()`:
- A leaf is Q-reducible only when it is a standard django-filter
Filter (no custom .filter() override, no method=, no distinct=). It
returns a plain `Q(field__lookup=value)` (negated for exclude
filters). Opaque operands -- label filters, repository_version,
href/prn/id resolvers, method filters, and ChoiceFilter (which
overrides .filter() for null handling) -- return None.
- NOT/AND/OR combine their children's Q objects with ~, &, | when all
children are reducible.
`evaluate()` then folds every reducible operand into a single flat Q and
applies only the genuinely opaque operands as pk__in subqueries. An
all-reducible expression -- however deeply nested -- collapses to one
flat SELECT; mixed expressions keep one subquery per opaque operand.
Opaque operands are always evaluated against the small base queryset, so
they never re-embed an accumulated filter chain.
Results are unchanged and the existing q filter functional tests
(test_q_filter_valid / test_q_filter_invalid, 21 cases) pass without
modification.
Benchmarks (100k rows, steady-state, count(), best of 4), old set-ops
vs. this change; SELECTs = number of SQL SELECT statements emitted:
Repositories (all-reducible name/number fields)
Expression set-ops this change
---------------------------------- -------------- --------------
name AND name 42 ms / 3 15 ms / 1
(A AND B) OR (C AND D) 79 ms / 7 20 ms / 1
(A AND (B OR C)) AND NOT D (cx 8) 144 ms / 8 19 ms / 1
name AND retain>=x AND retain<y 44 ms / 4 17 ms / 1
(num range) OR num=0 124 ms / 5 15 ms / 1
Tasks
name AND NOT name 68 ms / 4 17 ms / 1
name AND (NOT state OR state) 119 ms / 6 90 ms / 5
ContentGuards (name)
A AND B 56 ms / 3 19 ms / 1
(A AND B) OR C 155 ms / 5 20 ms / 1
Opaque-only expressions are unchanged: content repository_version
`X AND NOT Y` over 200k units stays ~2.45 s (199k pair) with no
regression, since both operands remain pk__in subqueries.
The speedup grows with nesting depth because the old plan added a
subquery per level while this one stays flat -- ~1.6x on a shallow OR up
to ~8x at the complexity-8 limit. The remaining opaque cases (e.g. task
`state`, a ChoiceFilter) could be reduced too in a follow-up.
Refs pulp#7831
Assisted by: Claude Opus 4.8
|
Following up on my own comment — I pushed a commit that I think actually addresses the performance issue. • Before: each operand of an The existing |
|
Some more benchmarks run on actual differences in code: 100k rows (200k for ContentGuard (name)
Repository (name / number)
Task
Content —
As the results show, the changes made in the new commit will in fact improve |
The
qcomplex filter composed its NOT/AND/OR operands with QuerySet .difference()/.intersection()/.union(), which compile to SQL EXCEPT/INTERSECT/UNION. For NOT in particular, the left operand is the full unfiltered queryset, so evaluating an expression likerepository_version=X AND NOT repository_version=Yseq-scans the entire content table and sorts/hashes every leg, spilling to disk on large repositories.Rewrite the three operand evaluators to compose with
pk__insubqueries:This collapses the plan to indexed anti-/semi-joins with no full-table scan, no SetOp, and no disk-spilling sorts. Results are unchanged (each operand still filters the same base queryset on a unique pk, so the implicit de-duplication of the set operations is preserved); the existing q filter functional tests pass without modification.
Benchmarked on a 200k-unit repository computing a 1000-unit diff between two repository versions (steady-state, work_mem=4MB):
Planning time also drops (e.g. ~1.4 ms -> ~0.5 ms) as the large inlined operand lists are replaced by parameterized subqueries.
Closes #7831
Assisted by: Claude Opus 4.8
📜 Checklist
See: Pull Request Walkthrough