Skip to content

Commit

Permalink
Fixed #18816 -- Removed "trim" argument from add_filter()
Browse files Browse the repository at this point in the history
The trim argument was used by split_exclude() only to trim the last
join from the given lookup. It is cleaner to just trim the last part
from the lookup in split_exclude() directly so that there is no need
to burden add_filter() with the logic needed for only split_exclude().
  • Loading branch information
akaariai committed Dec 16, 2012
1 parent d7b49f5 commit f811649
Showing 1 changed file with 32 additions and 24 deletions.
56 changes: 32 additions & 24 deletions django/db/models/sql/query.py
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -1029,7 +1029,7 @@ def add_aggregate(self, aggregate, model, alias, is_summary):
# Add the aggregate to the query # Add the aggregate to the query
aggregate.add_to_query(self, alias, col=col, source=source, is_summary=is_summary) aggregate.add_to_query(self, alias, col=col, source=source, is_summary=is_summary)


def add_filter(self, filter_expr, connector=AND, negate=False, trim=False, def add_filter(self, filter_expr, connector=AND, negate=False,
can_reuse=None, process_extras=True, force_having=False): can_reuse=None, process_extras=True, force_having=False):
""" """
Add a single filter to the query. The 'filter_expr' is a pair: Add a single filter to the query. The 'filter_expr' is a pair:
Expand All @@ -1042,9 +1042,6 @@ def add_filter(self, filter_expr, connector=AND, negate=False, trim=False,
should only happen once. So the caller is responsible for this (the should only happen once. So the caller is responsible for this (the
caller will normally be add_q(), so that as an example). caller will normally be add_q(), so that as an example).
If 'trim' is True, we automatically trim the final join group (used
internally when constructing nested queries).
If 'can_reuse' is a set, we are processing a component of a If 'can_reuse' is a set, we are processing a component of a
multi-component filter (e.g. filter(Q1, Q2)). In this case, 'can_reuse' multi-component filter (e.g. filter(Q1, Q2)). In this case, 'can_reuse'
will be a set of table aliases that can be reused in this filter, even will be a set of table aliases that can be reused in this filter, even
Expand Down Expand Up @@ -1115,7 +1112,7 @@ def add_filter(self, filter_expr, connector=AND, negate=False, trim=False,


opts = self.get_meta() opts = self.get_meta()
alias = self.get_initial_alias() alias = self.get_initial_alias()
allow_many = trim or not negate allow_many = not negate


try: try:
field, target, opts, join_list, last, extra_filters = self.setup_joins( field, target, opts, join_list, last, extra_filters = self.setup_joins(
Expand All @@ -1141,7 +1138,7 @@ def add_filter(self, filter_expr, connector=AND, negate=False, trim=False,
# Process the join list to see if we can remove any inner joins from # Process the join list to see if we can remove any inner joins from
# the far end (fewer tables in a query is better). # the far end (fewer tables in a query is better).
nonnull_comparison = (lookup_type == 'isnull' and value is False) nonnull_comparison = (lookup_type == 'isnull' and value is False)
col, alias, join_list = self.trim_joins(target, join_list, last, trim, col, alias, join_list = self.trim_joins(target, join_list, last,
nonnull_comparison) nonnull_comparison)


if connector == OR: if connector == OR:
Expand Down Expand Up @@ -1456,7 +1453,7 @@ def setup_joins(self, names, opts, alias, can_reuse, allow_many=True,


return field, target, opts, joins, last, extra_filters return field, target, opts, joins, last, extra_filters


def trim_joins(self, target, join_list, last, trim, nonnull_check=False): def trim_joins(self, target, join_list, last, nonnull_check=False):
""" """
Sometimes joins at the end of a multi-table sequence can be trimmed. If Sometimes joins at the end of a multi-table sequence can be trimmed. If
the final join is against the same column as we are comparing against, the final join is against the same column as we are comparing against,
Expand All @@ -1473,10 +1470,6 @@ def trim_joins(self, target, join_list, last, trim, nonnull_check=False):
same way, so 'last' has an entry for the first of the two tables and same way, so 'last' has an entry for the first of the two tables and
then the table immediately after the second table, in that case. then the table immediately after the second table, in that case.
The 'trim' parameter forces the final piece of the join list to be
trimmed before anything. See the documentation of add_filter() for
details about this.
The 'nonnull_check' parameter is True when we are using inner joins The 'nonnull_check' parameter is True when we are using inner joins
between tables explicitly to exclude NULL entries. In that case, the between tables explicitly to exclude NULL entries. In that case, the
tables shouldn't be trimmed, because the very action of joining to them tables shouldn't be trimmed, because the very action of joining to them
Expand All @@ -1489,16 +1482,7 @@ def trim_joins(self, target, join_list, last, trim, nonnull_check=False):
penultimate = last.pop() penultimate = last.pop()
if penultimate == final: if penultimate == final:
penultimate = last.pop() penultimate = last.pop()
if trim and final > 1: col = target.column
extra = join_list[penultimate:]
join_list = join_list[:penultimate]
final = penultimate
penultimate = last.pop()
col = self.alias_map[extra[0]].lhs_join_col
for alias in extra:
self.unref_alias(alias)
else:
col = target.column
alias = join_list[-1] alias = join_list[-1]
while final > 1: while final > 1:
join = self.alias_map[alias] join = self.alias_map[alias]
Expand All @@ -1520,6 +1504,19 @@ def split_exclude(self, filter_expr, prefix, can_reuse):
to use a subquery. This method constructs the nested query, given the to use a subquery. This method constructs the nested query, given the
original exclude filter (filter_expr) and the portion up to the first original exclude filter (filter_expr) and the portion up to the first
N-to-many relation field. N-to-many relation field.
As an example we could have original filter ~Q(child__name='foo').
We would get here with filter_expr = child_name, prefix = child and
can_reuse is a set of joins we can reuse for filtering in the original
query.
We will turn this into
WHERE pk NOT IN (SELECT parent_id FROM thetable
WHERE name = 'foo' AND parent_id IS NOT NULL)
It might be worth it to consider using WHERE NOT EXISTS as that has
saner null handling, and is easier for the backend's optimizer to
handle.
""" """
query = Query(self.model) query = Query(self.model)
query.add_filter(filter_expr) query.add_filter(filter_expr)
Expand All @@ -1532,8 +1529,19 @@ def split_exclude(self, filter_expr, prefix, can_reuse):
# nothing # nothing
alias, col = query.select[0].col alias, col = query.select[0].col
query.where.add((Constraint(alias, col, None), 'isnull', False), AND) query.where.add((Constraint(alias, col, None), 'isnull', False), AND)
# We need to trim the last part from the prefix.
trimmed_prefix = '__'.join(prefix.split(LOOKUP_SEP)[0:-1])

This comment has been minimized.

Copy link
@charettes

charettes Dec 17, 2012

Member

Is there a reason why you use LOOKUP_SEP for splitting but not for joining?

This comment has been minimized.

Copy link
@akaariai

akaariai Dec 17, 2012

Author Member

No reason, fixed.

if not trimmed_prefix:
rel, _, direct, m2m = self.model._meta.get_field_by_name(prefix)
if not m2m:
trimmed_prefix = rel.field.rel.field_name
else:
if direct:
trimmed_prefix = rel.m2m_target_field_name()
else:
trimmed_prefix = rel.field.m2m_reverse_target_field_name()


self.add_filter(('%s__in' % prefix, query), negate=True, trim=True, self.add_filter(('%s__in' % trimmed_prefix, query), negate=True,
can_reuse=can_reuse) can_reuse=can_reuse)


# If there's more than one join in the inner query (before any initial # If there's more than one join in the inner query (before any initial
Expand All @@ -1546,8 +1554,8 @@ def split_exclude(self, filter_expr, prefix, can_reuse):
active_positions = len([count for count active_positions = len([count for count
in query.alias_refcount.items() if count]) in query.alias_refcount.items() if count])
if active_positions > 1: if active_positions > 1:
self.add_filter(('%s__isnull' % prefix, False), negate=True, self.add_filter(('%s__isnull' % trimmed_prefix, False), negate=True,
trim=True, can_reuse=can_reuse) can_reuse=can_reuse)


def set_limits(self, low=None, high=None): def set_limits(self, low=None, high=None):
""" """
Expand Down

0 comments on commit f811649

Please sign in to comment.