feat: Add isin local execution impl #1993
Conversation
product-auto-label
bot
added
size: m
| @@ -1062,7 +1062,7 @@ def isin_op_impl(x: ibis_types.Value, op: ops.IsInOp): | |||
| if op.match_nulls and contains_nulls: | |||
| return x.isnull() | x.isin(matchable_ibis_values) | |||
| else: | |||
| return x.isin(matchable_ibis_values) | |||
| return x.isin(matchable_ibis_values).fillna(False) | |||
There was a problem hiding this comment.
Does this change any existing behavior? Were we missing a test case?
There was a problem hiding this comment.
hmm, it looks like we were handling this higher up. removed that code, and now the op itself is purely non-null. also added a test
| for val in expr.op.values: | ||
| # coercible, non-coercible | ||
| # float NaN/inf should be treated as distinct from 'true' null values | ||
| if cast(bool, pd.isna(val)) and not isinstance(val, float): |
There was a problem hiding this comment.
We can fold this into one statement:
match_nulls = cast(bool, pd.isna(val)) and not isinstance(val, float) and expr.op.match_nulls
There was a problem hiding this comment.
do to the elif statement, that wouldn't quite be the same
| pass | ||
|
|
||
| new_isin = ops.IsInOp(tuple(new_values), match_nulls=False).as_expr(arg) | ||
| if match_nulls: |
There was a problem hiding this comment.
So whether we match nulls partially depends on the last values in expr.op.values? Is that correct?
| arr = arr.rename_columns( | ||
| {old_name: new_names[i] for i, old_name in enumerate(col_ids)} | ||
| ) | ||
| assert_equivalence_execution(arr.node, REFERENCE_ENGINE, engine) |
There was a problem hiding this comment.
nit: empty line above assertion.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕