add scan tests with null values #1865
Conversation
|
|
||
| def test_scan_complements(catalog: InMemoryCatalog, arrow_table_with_null: pa.Table) -> None: | ||
| from pyiceberg.expressions.visitors import bind | ||
| from pyiceberg.io.pyarrow import _expression_to_complementary_pyarrow |
There was a problem hiding this comment.
_expression_to_complementary_pyarrow explicitly calls out null handling
iceberg-python/pyiceberg/io/pyarrow.py
Lines 883 to 886 in d69a191
| # this should be 2 | ||
| assert len(table.scan(row_filter="string != 'a'").to_arrow()) == 1 | ||
| assert len(table.scan(row_filter=NotEqualTo(term="string", literal=("a"))).to_arrow()) == 1 | ||
| assert len(table.scan(row_filter=Not(EqualTo(term="string", literal=("a")))).to_arrow()) == 1 | ||
| assert len(table.scan().to_arrow().filter(pc.field("string") != "a")) == 1 |
There was a problem hiding this comment.
@Fokko i think this might be a bug in how we handle nulls right now
There was a problem hiding this comment.
ugh, this gets really messy. for example, if string != 'a' should match null, what about ~(string != 'a')?
There was a problem hiding this comment.
Yes, this looks incorrect to me. ~(string != 'a') should be rewritten to string = 'a', this happens in rewrite_not.
There was a problem hiding this comment.
Another options is:
table.scan().to_arrow().filter(pc.coalesce(pc.field("string") != "a", pc.scalar(False))There was a problem hiding this comment.
Another option is to remove the not's in the beginning of the ScanPlan.
Rationale for this change
Test scan with null values
Similar to apache/iceberg-rust#1045
Are these changes tested?
Are there any user-facing changes?