Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGES/7831.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Optimized the `q` complex filter to compose `NOT`/`AND`/`OR` operands as flat `WHERE`
conditions in a single SQL statement for standard field filters, falling back to `pk__in`
subqueries only for opaque operands (label, `repository_version`, and href/prn/id resolvers).
This replaces the previous `EXCEPT`/`INTERSECT`/`UNION` set operations, avoiding full-table
scans and disk-spilling sorts, and keeps the query flat regardless of expression nesting depth.
79 changes: 72 additions & 7 deletions pulpcore/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -206,36 +206,101 @@ 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().
f = self.filter
if (
type(f).filter is not filters.Filter.filter
or getattr(f, "method", None) is not None
or getattr(f, "distinct", False)
):
return None
if self.value in EMPTY_VALUES:
return models.Q()
Comment on lines +221 to +222

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What role does "EMPTY_VALUES" play here?
Is it "the filter should not apply"? Would you just "not mention" a filter in "q=" if you didn't want it?
Or does it mean the value is NULL?

Could this mean we needed models.Q(**{f"{f.field_name}__isnull": not f.exclude})?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EMPTY_VALUES here means "the filter isn't applied". It's mirroring django_filters.Filter.filter, which returns the queryset unchanged for empty values. So the flat equivalent is an identity models.Q(), not IS NULL. We don't want to use __isnull for a few reasons:
• An empty/unspecified value shouldn't be reinterpreted as "field IS NULL" — that would change the meaning of key= vs. not mentioning key at all.
• The filters that do have null semantics override .filter(), so as_q() classifies them as opaque and returns None — they keep their own isnull handling on the evaluate() path. So the leaf reduction never needs to deal with NULL.

That said, you did catch something. as_q() was using the module-level EMPTY_VALUES, which we augment with the "null" sentinel. That meant name=null would collapse to a no-op (match-all) instead of matching the literal string "null" as it does on main. In the latest commit, I've fixed it to check django-filter's own EMPTY_VALUES so the leaf mirrors Filter.filter exactly. The "null" sentinel stays reserved for the null-aware filters that actually opt into it.

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()
if q is not None:
return qs.filter(q)
return self.filter.filter(qs, self.value)

class _NotAction:
def __init__(self, tokens):
self.expr = tokens[0][0]
self.complexity = self.expr.complexity + 1

def as_q(self):
child = self.expr.as_q()
return None if child is None else ~child

def evaluate(self, qs):
return qs.difference(self.expr.evaluate(qs))
q = self.as_q()
if q is not None:
return qs.filter(q)
# Opaque operand: anti-join via a NOT IN subquery.
return qs.exclude(pk__in=self.expr.evaluate(qs).values("pk"))

class _AndAction:
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]
if any(child is None for child in children):
return None
query = models.Q()
for child in children:
query &= child
return query

def evaluate(self, qs):
return (
self.exprs[0]
.evaluate(qs)
.intersection(*[expr.evaluate(qs) for expr in self.exprs[1:]])
)
# 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()
if child is not None:
combined &= child
has_q = True
else:
result = result.filter(pk__in=expr.evaluate(qs).values("pk"))
if has_q:
result = result.filter(combined)
return result

class _OrAction:
def __init__(self, tokens):
self.exprs = tokens[0]
self.complexity = sum((expr.complexity for expr in self.exprs)) + 1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def as_q(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now every subexpression is tried with as_q() on succeed we are done, if not, we fall back to evaluate. I wish these two methods had names reflecting that.

Ideas:
evaluate_q -> evaluate_opaque
as_q -> as_opaque
evaluate_flat -> evalueate_complex
evaluate_optimized -> evaluate

The catch probably is that the outermost is always called by evaluate, right?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - thanks for pointing this out. I've renamed as_q to as_flat_q and kept evaluate as the general apply method, plus added docstrings to make the protocol explicit.

And yes, evaluate() is the sole entry point; ExpressionFilter.filter calls value.evaluate(qs) on the outermost node, which then recurses. I also updated the docstring on ExpressionFilterField spelling out that two-method protocol so it's a bit clearer on what it does.

children = [expr.as_q() for expr in self.exprs]
if any(child is None for child in children):
return None
query = children[0]
for child in children[1:]:
query |= child
return query

def evaluate(self, qs):
return self.exprs[0].evaluate(qs).union(*[expr.evaluate(qs) for expr in self.exprs[1:]])
# 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()
if child is not None:
query |= child
else:
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
Expand Down
Loading