Skip to content

Commit

Permalink
fix(ir): avoid deduplicating filters based solely on their name (#9476)
Browse files Browse the repository at this point in the history
This PR fixes an issue where we were using `unwrap_aliases` to
deduplicate and
enforce expressions by their name in `filter`s.

That approach makes sense in the context of projections, but the name of
predicate expressions in a `filter` is irrelevant.

The solution was to continue to unwrap aliases but avoid deduplicating
expressions based on their name, and do it solely on the hash value of
expressions.

Closes #9474.
  • Loading branch information
cpcloud authored Jul 1, 2024
1 parent adade5e commit b35582e
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 2 deletions.
11 changes: 11 additions & 0 deletions ibis/expr/tests/test_newrels.py
Original file line number Diff line number Diff line change
Expand Up @@ -1737,3 +1737,14 @@ def test_projections_with_different_field_order_are_unequal():
t2 = t.select(b=2, a=1)

assert not t1.equals(t2)


def test_filters_are_allowed_to_have_the_same_name():
t = ibis.table({"a": "int64", "b": "string"}, name="t")
f1 = t.filter(t.a > 1, t.a > 1)
f2 = t.filter(t.a > 1)
f3 = t.filter((t.a > 1).name("a"))
f4 = t.filter((t.a > 1).name("a"), (t.a > 1).name("b"))
assert f1.equals(f2)
assert f1.equals(f3)
assert f1.equals(f4)
17 changes: 15 additions & 2 deletions ibis/expr/types/relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -2542,8 +2542,21 @@ def filter(
from ibis.expr.rewrites import flatten_predicates, rewrite_filter_input

preds = self.bind(*predicates)
preds = unwrap_aliases(preds)
preds = flatten_predicates(list(preds.values()))

# we can't use `unwrap_aliases` here because that function
# deduplicates based on name alone
#
# it's perfectly valid to repeat a filter, even if it might be
# useless, so enforcing uniquely named expressions here doesn't make
# sense
#
# instead, compute all distinct unaliased predicates
result = toolz.unique(
node.arg if isinstance(node := value.op(), ops.Alias) else node
for value in preds
)

preds = flatten_predicates(list(result))
preds = list(map(rewrite_filter_input, preds))
if not preds:
raise com.IbisInputError("You must pass at least one predicate to filter")
Expand Down

0 comments on commit b35582e

Please sign in to comment.