From 9386abb480f5b9a6445318a16b41aa3fc72c1ada Mon Sep 17 00:00:00 2001 From: Andrew Cheng Date: Tue, 7 Jul 2026 19:05:41 +0000 Subject: [PATCH 1/3] Optimize q filter to use pk__in subqueries instead of set operations 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 #7831 Assisted by: Claude Opus 4.8 --- CHANGES/7831.bugfix | 3 +++ pulpcore/filters.py | 16 +++++++++------- 2 files changed, 12 insertions(+), 7 deletions(-) create mode 100644 CHANGES/7831.bugfix diff --git a/CHANGES/7831.bugfix b/CHANGES/7831.bugfix new file mode 100644 index 00000000000..8fa0b649419 --- /dev/null +++ b/CHANGES/7831.bugfix @@ -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 diff --git a/pulpcore/filters.py b/pulpcore/filters.py index e225769a5c0..bcb99b5cd4f 100644 --- a/pulpcore/filters.py +++ b/pulpcore/filters.py @@ -215,7 +215,7 @@ def __init__(self, tokens): self.complexity = self.expr.complexity + 1 def evaluate(self, qs): - return qs.difference(self.expr.evaluate(qs)) + return qs.exclude(pk__in=self.expr.evaluate(qs).values("pk")) class _AndAction: def __init__(self, tokens): @@ -223,11 +223,10 @@ def __init__(self, tokens): self.complexity = sum((expr.complexity for expr in self.exprs)) + 1 def evaluate(self, qs): - return ( - self.exprs[0] - .evaluate(qs) - .intersection(*[expr.evaluate(qs) for expr in self.exprs[1:]]) - ) + result = qs + for expr in self.exprs: + result = result.filter(pk__in=expr.evaluate(qs).values("pk")) + return result class _OrAction: def __init__(self, tokens): @@ -235,7 +234,10 @@ def __init__(self, tokens): self.complexity = sum((expr.complexity for expr in self.exprs)) + 1 def evaluate(self, qs): - return self.exprs[0].evaluate(qs).union(*[expr.evaluate(qs) for expr in self.exprs[1:]]) + query = models.Q() + for expr in self.exprs: + query |= models.Q(pk__in=expr.evaluate(qs).values("pk")) + return qs.filter(query) def __init__(self, *args, **kwargs): self.filterset = kwargs.pop("filter").parent From 1d6a98fe7939319966bb6aaa74204c0f22194863 Mon Sep 17 00:00:00 2001 From: Andrew Cheng Date: Wed, 8 Jul 2026 17:24:15 +0000 Subject: [PATCH 2/3] Flatten q filter operands into a single WHERE where possible 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 Date: Fri, 17 Jul 2026 22:00:46 +0000 Subject: [PATCH 3/3] Address function naming concerns and clarify EMPTY_VALUES --- pulpcore/filters.py | 67 +++++++++++++++++++++++++++++++++------------ 1 file changed, 49 insertions(+), 18 deletions(-) diff --git a/pulpcore/filters.py b/pulpcore/filters.py index 1f38785e720..3aaa5626681 100644 --- a/pulpcore/filters.py +++ b/pulpcore/filters.py @@ -10,7 +10,7 @@ from django.db import models from django.forms.utils import ErrorList from django.urls import Resolver404, resolve -from django_filters.constants import EMPTY_VALUES +from django_filters.constants import EMPTY_VALUES as STANDARD_EMPTY_VALUES from django_filters.rest_framework import BaseInFilter, DjangoFilterBackend, filters, filterset from drf_spectacular.contrib.django_filters import DjangoFilterExtension from drf_spectacular.plumbing import build_basic_type @@ -20,7 +20,7 @@ from pulpcore.app.util import extract_pk, get_domain_pk, resolve_prn -EMPTY_VALUES = (*EMPTY_VALUES, "null") +EMPTY_VALUES = (*STANDARD_EMPTY_VALUES, "null") UPMatch = namedtuple("UPMatch", ("model", "pk")) @@ -185,6 +185,23 @@ class PulpTypeInFilter(BaseInFilter, PulpTypeFilter): class ExpressionFilterField(forms.CharField): + """Parse and compile a ``q=`` complex-filter expression into a queryset transform. + + Each parsed node (leaf ``_FilterAction`` and the ``NOT``/``AND``/``OR`` combinators) implements + a two-method protocol: + + * ``as_flat_q()`` -> ``Q | None``: a pure, side-effect-free attempt to represent the *whole* + subtree as a single flat ``Q`` object. It returns ``None`` if any operand is "opaque" (a + filter with a custom ``.filter()`` override, a ``method``, or ``.distinct()``) and therefore + cannot be expressed as a plain ``WHERE`` condition. + * ``evaluate(qs)`` -> ``QuerySet``: applies the subtree to ``qs``. It uses ``as_flat_q()`` when + the subtree is fully reducible (one flat ``WHERE``) and otherwise falls back to composing the + opaque operands as ``pk__in`` semi-/anti-join subqueries. + + ``evaluate()`` is the sole external entry point: :meth:`ExpressionFilter.filter` calls + ``value.evaluate(qs)`` on the outermost node. + """ + class _FilterAction: def __init__(self, filterset, tokens): key = tokens[0].key @@ -206,11 +223,14 @@ def __init__(self, filterset, tokens): self.value = form.cleaned_data[key] self.complexity = 1 - def as_q(self): - # A leaf reduces to a plain WHERE condition (Q) only when it is a standard - # django-filter Filter: no custom .filter() override, no method, no .distinct(). - # Anything else (href/prn resolvers, repository_version, method filters, ...) is - # opaque and must be applied via its own queryset transform in evaluate(). + def as_flat_q(self): + """Return this leaf as a flat ``Q``, or ``None`` if the filter is opaque. + + A leaf reduces to a plain ``WHERE`` condition only when it is a standard django-filter + ``Filter``: no custom ``.filter()`` override, no ``method``, and no ``.distinct()``. + Anything else (href/prn resolvers, ``repository_version``, method filters, ...) is + opaque and must be applied via its own queryset transform in :meth:`evaluate`. + """ f = self.filter if ( type(f).filter is not filters.Filter.filter @@ -218,13 +238,18 @@ def as_q(self): or getattr(f, "distinct", False) ): return None - if self.value in EMPTY_VALUES: + # Mirror filters.Filter.filter exactly: an empty/unspecified value means the filter is + # not applied (identity Q), not "IS NULL". Use django-filter's own EMPTY_VALUES rather + # than the module-augmented set so the sentinel "null" is not swallowed here; null-aware + # filters override .filter() and are handled on the opaque evaluate() path above. + if self.value in STANDARD_EMPTY_VALUES: return models.Q() q = models.Q(**{f"{f.field_name}__{f.lookup_expr}": self.value}) return ~q if f.exclude else q def evaluate(self, qs): - q = self.as_q() + """Apply the leaf: a flat ``Q`` when reducible, else the filter's own transform.""" + q = self.as_flat_q() if q is not None: return qs.filter(q) return self.filter.filter(qs, self.value) @@ -234,12 +259,14 @@ def __init__(self, tokens): self.expr = tokens[0][0] self.complexity = self.expr.complexity + 1 - def as_q(self): - child = self.expr.as_q() + def as_flat_q(self): + """Negate the child's flat ``Q``; ``None`` if the child is opaque.""" + child = self.expr.as_flat_q() return None if child is None else ~child def evaluate(self, qs): - q = self.as_q() + """Apply ``NOT``: negate a flat ``Q`` when reducible, else anti-join the operand.""" + q = self.as_flat_q() if q is not None: return qs.filter(q) # Opaque operand: anti-join via a NOT IN subquery. @@ -250,8 +277,9 @@ def __init__(self, tokens): self.exprs = tokens[0] self.complexity = sum((expr.complexity for expr in self.exprs)) + 1 - def as_q(self): - children = [expr.as_q() for expr in self.exprs] + def as_flat_q(self): + """AND every child's flat ``Q`` into one; ``None`` if any child is opaque.""" + children = [expr.as_flat_q() for expr in self.exprs] if any(child is None for child in children): return None query = models.Q() @@ -260,13 +288,14 @@ def as_q(self): return query def evaluate(self, qs): + """Apply ``AND``: fold reducible operands into one flat ``WHERE``, semi-join the rest.""" # Fold every Q-reducible operand into a single flat WHERE, and apply only the # genuinely opaque operands as pk__in semi-join subqueries. result = qs combined = models.Q() has_q = False for expr in self.exprs: - child = expr.as_q() + child = expr.as_flat_q() if child is not None: combined &= child has_q = True @@ -281,8 +310,9 @@ def __init__(self, tokens): self.exprs = tokens[0] self.complexity = sum((expr.complexity for expr in self.exprs)) + 1 - def as_q(self): - children = [expr.as_q() for expr in self.exprs] + def as_flat_q(self): + """OR every child's flat ``Q`` into one; ``None`` if any child is opaque.""" + children = [expr.as_flat_q() for expr in self.exprs] if any(child is None for child in children): return None query = children[0] @@ -291,11 +321,12 @@ def as_q(self): return query def evaluate(self, qs): + """Apply ``OR``: reducible operands stay flat, opaque ones contribute ``pk__in``.""" # Reducible operands stay as flat OR'd conditions; opaque operands contribute a # pk__in subquery. Both are combined in a single WHERE clause. query = models.Q() for expr in self.exprs: - child = expr.as_q() + child = expr.as_flat_q() if child is not None: query |= child else: