syun64 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1687978956
##########
tests/io/test_pyarrow_visitor.py:
##########
@@ -580,3 +590,57 @@ def
test_pyarrow_schema_ensure_large_types(pyarrow_schema_nested_without_ids: pa
),
])
assert
_pyarrow_schema_ensure_large_types(pyarrow_schema_nested_without_ids) ==
expected_schema
+
+
+@pytest.fixture
+def bound_reference_long() -> BoundReference[int]:
+ return BoundReference(field=NestedField(1, "field", LongType(),
required=False), accessor=Accessor(position=0, inner=None))
+
+
+def test_collect_null_unmentioned_terms() -> None:
+ bound_reference_str = BoundReference(
+ field=NestedField(1, "field_str", StringType(), required=False),
accessor=Accessor(position=0, inner=None)
+ )
+ bound_eq_str_field = BoundEqualTo(term=bound_reference_str,
literal=literal("hello"))
+
+ bound_reference_long = BoundReference(
+ field=NestedField(2, "field_long", LongType(), required=False),
accessor=Accessor(position=1, inner=None)
+ )
+ bound_larger_than_long_field = BoundGreaterThan(term=bound_reference_long,
literal=literal(100)) # type: ignore
+
+ bound_reference_bool = BoundReference(
+ field=NestedField(3, "field_bool", BooleanType(), required=False),
accessor=Accessor(position=2, inner=None)
+ )
+ bound_is_null_bool_field = BoundIsNull(bound_reference_bool)
+
+ bound_expr = Or(And(bound_eq_str_field, bound_larger_than_long_field),
bound_is_null_bool_field)
+
+ categorized_terms = _get_null_nan_refs(bound_expr)
+ assert {"field_long", "field_str"} == {f.field.name for f in
categorized_terms[0]}
+ assert {
+ "field_bool",
+ } == {f.field.name for f in categorized_terms[1]}
+
+
+def
test_collect_null_unmentioned_terms_with_multiple_predicates_on_the_same_term()
-> None:
+ """Test a single term appears multiple places in the expression tree"""
+ bound_reference_str = BoundReference(
+ field=NestedField(1, "field_str", StringType(), required=False),
accessor=Accessor(position=0, inner=None)
+ )
+ bound_eq_str_field = BoundEqualTo(term=bound_reference_str,
literal=literal("hello"))
+
+ bound_reference_long = BoundReference(
+ field=NestedField(1, "field_long", LongType(), required=False),
accessor=Accessor(position=1, inner=None)
+ )
+ bound_larger_than_long_field = BoundGreaterThan(term=bound_reference_long,
literal=literal(100)) # type: ignore
+
+ bound_is_null_long_field = BoundIsNull(bound_reference_long)
+
+ bound_expr = Or(
+ And(Or(And(bound_eq_str_field, bound_larger_than_long_field),
bound_is_null_long_field), bound_larger_than_long_field),
+ bound_eq_str_field,
+ )
+
+ categorized_terms = _get_null_nan_refs(bound_expr)
+ assert {"field_str"} == set({f.field.name for f in categorized_terms[0]})
Review Comment:
This is a nit, but I think it would be nice to use the convention of
`assert actual == expected` as to align with rest of the practices in the
repository - it will be easier to investigate failed assertions on first glance
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
