You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "jqin61 (via GitHub)" <gi...@apache.org> on 2024/07/22 20:02:22 UTC
[PR] fix table.delete()/overwrite() with null values [iceberg-python]
jqin61 opened a new pull request, #955:
URL: https://github.com/apache/iceberg-python/pull/955
for issue https://github.com/apache/iceberg-python/issues/954
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "jqin61 (via GitHub)" <gi...@apache.org>.
jqin61 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1690408866
##########
tests/integration/test_deletes.py:
##########
@@ -417,3 +452,107 @@ def test_delete_truncate(session_catalog: RestCatalog) -> None:
assert len(entries) == 1
assert entries[0].status == ManifestEntryStatus.DELETED
+
+
+@pytest.mark.integration
+def test_delete_overwrite_table_with_null(session_catalog: RestCatalog) -> None:
+ arrow_schema = pa.schema([pa.field("ints", pa.int32())])
+ arrow_tbl = pa.Table.from_pylist(
+ [{"ints": 1}, {"ints": 2}, {"ints": None}],
+ schema=arrow_schema,
+ )
+
+ iceberg_schema = Schema(NestedField(1, "ints", IntegerType()))
+
+ tbl_identifier = "default.test_delete_overwrite_with_null"
+
+ try:
+ session_catalog.drop_table(tbl_identifier)
+ except NoSuchTableError:
+ pass
+
+ tbl = session_catalog.create_table(tbl_identifier, iceberg_schema)
+ tbl.append(arrow_tbl)
Review Comment:
sounds good! let's put it in a future pr
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "jqin61 (via GitHub)" <gi...@apache.org>.
jqin61 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1690328141
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,152 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _NullNaNUnmentionedTermsCollector(BoundBooleanExpressionVisitor[Any]):
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ is_null_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ null_unmentioned_bound_terms: set[BoundTerm[Any]]
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ is_nan_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ nan_unmentioned_bound_terms: set[BoundTerm[Any]]
+
+ def __init__(self) -> None:
+ self.is_null_or_not_bound_terms = set()
+ self.null_unmentioned_bound_terms = set()
+ self.is_nan_or_not_bound_terms = set()
+ self.nan_unmentioned_bound_terms = set()
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
Review Comment:
let me check if i need to add tests for this
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "jqin61 (via GitHub)" <gi...@apache.org>.
jqin61 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1689900531
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,165 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _NullNaNUnmentionedTermsCollector(BoundBooleanExpressionVisitor[Any]):
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ is_null_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ null_unmentioned_bound_terms: set[BoundTerm[Any]]
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ is_nan_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ nan_unmentioned_bound_terms: set[BoundTerm[Any]]
+
+ def __init__(self) -> None:
+ self.is_null_or_not_bound_terms = set()
+ self.null_unmentioned_bound_terms = set()
+ self.is_nan_or_not_bound_terms = set()
+ self.nan_unmentioned_bound_terms = set()
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def collect(
+ self,
+ expr: BooleanExpression,
+ ) -> None:
+ """Collect the bound references categorized by having at least one is_null or is_not_null in the expr and the remaining."""
+ boolean_expression_visit(expr, self)
+
+
def expression_to_pyarrow(expr: BooleanExpression) -> pc.Expression:
return boolean_expression_visit(expr, _ConvertToArrowExpression())
+def _expression_to_complementary_pyarrow(expr: BooleanExpression) -> pc.Expression:
+ """Complementary filter conversion function of expression_to_pyarrow.
+
+ Could not use expression_to_pyarrow(Not(expr)) to achieve this complementary effect because ~ in pyarrow.compute.Expression does not handle null.
+ """
+ collector = _NullNaNUnmentionedTermsCollector()
+ collector.collect(expr)
+
+ def _downcast_term_to_reference(bound_terms: Set[BoundTerm[Any]]) -> Set[BoundReference[Any]]:
Review Comment:
ah i see, BoundPredicate's interface is BoundTerm, so it is already upcast. I was creating non-existing trouble for myself. Thanks a lot for the catch!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1690505567
##########
pyiceberg/io/pyarrow.py:
##########
@@ -576,11 +572,11 @@ def _convert_scalar(value: Any, iceberg_type: IcebergType) -> pa.scalar:
class _ConvertToArrowExpression(BoundBooleanExpressionVisitor[pc.Expression]):
- def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> pc.Expression:
+ def visit_in(self, term: BoundTerm[L], literals: Set[L]) -> pc.Expression:
Review Comment:
Thanks for spotting this. I think these are missed from the last major typing fix of visitors:
- https://github.com/apache/iceberg/pull/6258#discussion_r1034007197
- https://github.com/apache/iceberg/pull/6308/files#diff-49144c27eab7e03926b0310aa8b513dbeb9c8d2a0d33bacf8dedbd88b4680aac
Given that this is in a concrete class, how about we just use `Any` to indicate that we accept general terms?
```suggestion
def visit_in(self, term: BoundTerm[Any], literals: Set[Any]) -> pc.Expression:
```
which is also consistent with the type in other methods.
##########
pyiceberg/io/pyarrow.py:
##########
@@ -576,11 +572,11 @@ def _convert_scalar(value: Any, iceberg_type: IcebergType) -> pa.scalar:
class _ConvertToArrowExpression(BoundBooleanExpressionVisitor[pc.Expression]):
- def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> pc.Expression:
+ def visit_in(self, term: BoundTerm[L], literals: Set[L]) -> pc.Expression:
pyarrow_literals = pa.array(literals, type=schema_to_pyarrow(term.ref().field.field_type))
return pc.field(term.ref().field.name).isin(pyarrow_literals)
- def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> pc.Expression:
+ def visit_not_in(self, term: BoundTerm[L], literals: Set[L]) -> pc.Expression:
Review Comment:
```suggestion
def visit_not_in(self, term: BoundTerm[Any], literals: Set[Any]) -> pc.Expression:
```
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,152 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _NullNaNUnmentionedTermsCollector(BoundBooleanExpressionVisitor[None]):
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ is_null_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ null_unmentioned_bound_terms: set[BoundTerm[Any]]
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ is_nan_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ nan_unmentioned_bound_terms: set[BoundTerm[Any]]
+
+ def __init__(self) -> None:
+ super().__init__()
+ self.is_null_or_not_bound_terms = set()
+ self.null_unmentioned_bound_terms = set()
+ self.is_nan_or_not_bound_terms = set()
+ self.nan_unmentioned_bound_terms = set()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[L], literals: Set[L]) -> None:
Review Comment:
```suggestion
def visit_in(self, term: BoundTerm[Any], literals: Set[Any]) -> None:
```
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,152 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _NullNaNUnmentionedTermsCollector(BoundBooleanExpressionVisitor[None]):
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ is_null_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ null_unmentioned_bound_terms: set[BoundTerm[Any]]
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ is_nan_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ nan_unmentioned_bound_terms: set[BoundTerm[Any]]
+
+ def __init__(self) -> None:
+ super().__init__()
+ self.is_null_or_not_bound_terms = set()
+ self.null_unmentioned_bound_terms = set()
+ self.is_nan_or_not_bound_terms = set()
+ self.nan_unmentioned_bound_terms = set()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[L], literals: Set[L]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_in(self, term: BoundTerm[L], literals: Set[L]) -> None:
Review Comment:
```suggestion
def visit_not_in(self, term: BoundTerm[Any], literals: Set[Any]) -> None:
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "jqin61 (via GitHub)" <gi...@apache.org>.
jqin61 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1688687715
##########
tests/io/test_pyarrow_visitor.py:
##########
@@ -580,3 +594,127 @@ 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_str() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(1, "field_str_unmentioned", StringType(), required=False), accessor=Accessor(position=0, inner=None)
+ )
+
+
+@pytest.fixture
+def bound_reference_float() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(2, "field_float_mentioned_nan", FloatType(), required=False), accessor=Accessor(position=1, inner=None)
+ )
+
+
+@pytest.fixture
+def bound_reference_double() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(3, "field_double_mentioned_null", DoubleType(), required=False),
+ accessor=Accessor(position=2, inner=None),
+ )
+
+
+@pytest.fixture
+def bound_eq_str_field(bound_reference_str: BoundReference[Any]) -> BoundEqualTo[Any]:
+ return BoundEqualTo(term=bound_reference_str, literal=literal("hello"))
+
+
+@pytest.fixture
+def bound_greater_than_float_field(bound_reference_float: BoundReference[Any]) -> BoundGreaterThan[Any]:
+ return BoundGreaterThan(term=bound_reference_float, literal=literal(100))
+
+
+@pytest.fixture
+def bound_is_nan_float_field(bound_reference_float: BoundReference[Any]) -> BoundIsNaN[Any]:
+ return BoundIsNaN(bound_reference_float)
+
+
+@pytest.fixture
+def bound_eq_double_field(bound_reference_double: BoundReference[Any]) -> BoundEqualTo[Any]:
+ return BoundEqualTo(term=bound_reference_double, literal=literal(False))
+
+
+@pytest.fixture
+def bound_is_null_double_field(bound_reference_double: BoundReference[Any]) -> BoundIsNull[Any]:
+ return BoundIsNull(bound_reference_double)
+
+
+def test_collect_null_nan_unmentioned_terms(
+ bound_eq_str_field: BoundEqualTo[Any], bound_is_nan_float_field: BoundIsNaN[Any], bound_is_null_double_field: BoundIsNull[Any]
+) -> None:
+ bound_expr = And(
+ Or(And(bound_eq_str_field, bound_is_nan_float_field), bound_is_null_double_field), Not(bound_is_nan_float_field)
+ )
+ collector = _NullNaNUnmentionedTermsCollector()
+ collector.collect(bound_expr)
+ assert {f.field.name for f in collector.null_unmentioned_bound_terms} == { # type: ignore
Review Comment:
This type: ignore is due to the similar issue of Term v.s. Reference I mentioned above because only Reference has .field
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "sungwy (via GitHub)" <gi...@apache.org>.
sungwy merged PR #955:
URL: https://github.com/apache/iceberg-python/pull/955
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "jqin61 (via GitHub)" <gi...@apache.org>.
jqin61 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1688687715
##########
tests/io/test_pyarrow_visitor.py:
##########
@@ -580,3 +594,127 @@ 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_str() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(1, "field_str_unmentioned", StringType(), required=False), accessor=Accessor(position=0, inner=None)
+ )
+
+
+@pytest.fixture
+def bound_reference_float() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(2, "field_float_mentioned_nan", FloatType(), required=False), accessor=Accessor(position=1, inner=None)
+ )
+
+
+@pytest.fixture
+def bound_reference_double() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(3, "field_double_mentioned_null", DoubleType(), required=False),
+ accessor=Accessor(position=2, inner=None),
+ )
+
+
+@pytest.fixture
+def bound_eq_str_field(bound_reference_str: BoundReference[Any]) -> BoundEqualTo[Any]:
+ return BoundEqualTo(term=bound_reference_str, literal=literal("hello"))
+
+
+@pytest.fixture
+def bound_greater_than_float_field(bound_reference_float: BoundReference[Any]) -> BoundGreaterThan[Any]:
+ return BoundGreaterThan(term=bound_reference_float, literal=literal(100))
+
+
+@pytest.fixture
+def bound_is_nan_float_field(bound_reference_float: BoundReference[Any]) -> BoundIsNaN[Any]:
+ return BoundIsNaN(bound_reference_float)
+
+
+@pytest.fixture
+def bound_eq_double_field(bound_reference_double: BoundReference[Any]) -> BoundEqualTo[Any]:
+ return BoundEqualTo(term=bound_reference_double, literal=literal(False))
+
+
+@pytest.fixture
+def bound_is_null_double_field(bound_reference_double: BoundReference[Any]) -> BoundIsNull[Any]:
+ return BoundIsNull(bound_reference_double)
+
+
+def test_collect_null_nan_unmentioned_terms(
+ bound_eq_str_field: BoundEqualTo[Any], bound_is_nan_float_field: BoundIsNaN[Any], bound_is_null_double_field: BoundIsNull[Any]
+) -> None:
+ bound_expr = And(
+ Or(And(bound_eq_str_field, bound_is_nan_float_field), bound_is_null_double_field), Not(bound_is_nan_float_field)
+ )
+ collector = _NullNaNUnmentionedTermsCollector()
+ collector.collect(bound_expr)
+ assert {f.field.name for f in collector.null_unmentioned_bound_terms} == { # type: ignore
Review Comment:
This type: ignore is due to the similar issue of term v.s. reference I mentioned above because only reference has .field
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1690176614
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,152 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _NullNaNUnmentionedTermsCollector(BoundBooleanExpressionVisitor[Any]):
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ is_null_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ null_unmentioned_bound_terms: set[BoundTerm[Any]]
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ is_nan_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ nan_unmentioned_bound_terms: set[BoundTerm[Any]]
+
+ def __init__(self) -> None:
+ self.is_null_or_not_bound_terms = set()
+ self.null_unmentioned_bound_terms = set()
+ self.is_nan_or_not_bound_terms = set()
+ self.nan_unmentioned_bound_terms = set()
+ super().__init__()
Review Comment:
[nit] I prefer to move `super().__init__()` to the beginning of `__init__` if it is not required to initialize child class fields before parent class's
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,152 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _NullNaNUnmentionedTermsCollector(BoundBooleanExpressionVisitor[Any]):
Review Comment:
```suggestion
class _NullNaNUnmentionedTermsCollector(BoundBooleanExpressionVisitor[None]):
```
How about specifying `None` here? The return type for this visitor is None
##########
tests/integration/test_deletes.py:
##########
@@ -417,3 +452,107 @@ def test_delete_truncate(session_catalog: RestCatalog) -> None:
assert len(entries) == 1
assert entries[0].status == ManifestEntryStatus.DELETED
+
+
+@pytest.mark.integration
+def test_delete_overwrite_table_with_null(session_catalog: RestCatalog) -> None:
+ arrow_schema = pa.schema([pa.field("ints", pa.int32())])
+ arrow_tbl = pa.Table.from_pylist(
+ [{"ints": 1}, {"ints": 2}, {"ints": None}],
+ schema=arrow_schema,
+ )
+
+ iceberg_schema = Schema(NestedField(1, "ints", IntegerType()))
+
+ tbl_identifier = "default.test_delete_overwrite_with_null"
+
+ try:
+ session_catalog.drop_table(tbl_identifier)
+ except NoSuchTableError:
+ pass
+
+ tbl = session_catalog.create_table(tbl_identifier, iceberg_schema)
+ tbl.append(arrow_tbl)
Review Comment:
[Optional] I like the `_create_table` util we have in other integration tests:
https://github.com/apache/iceberg-python/blob/1ed3abdd1aec480911eeec4f0f46a04efe53dc06/tests/integration/test_add_files.py#L121-L138
It will be great if we can have the same for `test_deletes`. (totally ok to be done in a follow-up PR after the release)
In the future, we may also want to consider unifying the behavior of `_create_table` across different tests and moving it to `conftest.py`. We can do this in a follow-up PR.
##########
tests/io/test_pyarrow.py:
##########
@@ -15,7 +15,6 @@
# specific language governing permissions and limitations
# under the License.
# pylint: disable=protected-access,unused-argument,redefined-outer-name
-
Review Comment:
[Nit] Could you please revert this?
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,152 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _NullNaNUnmentionedTermsCollector(BoundBooleanExpressionVisitor[Any]):
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ is_null_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ null_unmentioned_bound_terms: set[BoundTerm[Any]]
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ is_nan_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ nan_unmentioned_bound_terms: set[BoundTerm[Any]]
+
+ def __init__(self) -> None:
+ self.is_null_or_not_bound_terms = set()
+ self.null_unmentioned_bound_terms = set()
+ self.is_nan_or_not_bound_terms = set()
+ self.nan_unmentioned_bound_terms = set()
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
Review Comment:
The type of `_result` fields seems off here. In this case, I think we always expect to receive `None` from child/left/right
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "jqin61 (via GitHub)" <gi...@apache.org>.
jqin61 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1690328141
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,152 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _NullNaNUnmentionedTermsCollector(BoundBooleanExpressionVisitor[Any]):
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ is_null_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ null_unmentioned_bound_terms: set[BoundTerm[Any]]
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ is_nan_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ nan_unmentioned_bound_terms: set[BoundTerm[Any]]
+
+ def __init__(self) -> None:
+ self.is_null_or_not_bound_terms = set()
+ self.null_unmentioned_bound_terms = set()
+ self.is_nan_or_not_bound_terms = set()
+ self.nan_unmentioned_bound_terms = set()
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
Review Comment:
let me check if i need to add tests for this
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1687162697
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,124 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _CollectNullUnmentionedTermsFromExpression(BoundBooleanExpressionVisitor[Any]):
+ def __init__(self) -> None:
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ self.is_valid_or_not_bound_terms: set[BoundTerm[Any]] = set()
+ # The remaining BoundTerms appearing in the boolean expr.
+ self.null_unmentioned_bound_terms: set[BoundTerm[Any]] = set()
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_valid_or_not_bound_terms.add(term)
+
+ def _handle_skipped(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_valid_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_skipped(term)
+
+ # todo: do I have to modify this as well
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_skipped(term)
+
+ # todo: do I have to modify this as well, might need 2 self.xx sets for mentioned_nan and none-mentioned-nan
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+
+def _get_is_valid_or_not_bound_refs(expr: BooleanExpression) -> tuple[Set[BoundReference[Any]], Set[BoundReference[Any]]]:
+ """Collect the bound terms catogorized by having at least one is_null or is_not_null in the expr and the remaining."""
+ collector = _CollectNullUnmentionedTermsFromExpression()
+ boolean_expression_visit(expr, collector)
+ null_unmentioned_bound_terms = collector.null_unmentioned_bound_terms
+ is_valid_or_not_bound_terms = collector.is_valid_or_not_bound_terms
+
+ null_unmentioned_bound_refs: Set[BoundReference[Any]] = set()
+ is_valid_or_not_bound_refs: Set[BoundReference[Any]] = set()
+ for t in null_unmentioned_bound_terms:
+ if not isinstance(t, BoundReference):
+ raise ValueError("Collected Bound Term that is not reference.")
+ else:
+ null_unmentioned_bound_refs.add(t)
+ for t in is_valid_or_not_bound_terms:
+ if not isinstance(t, BoundReference):
+ raise ValueError("Collected Bound Term that is not reference.")
+ else:
+ is_valid_or_not_bound_refs.add(t)
+ return null_unmentioned_bound_refs, is_valid_or_not_bound_refs
+
+
def expression_to_pyarrow(expr: BooleanExpression) -> pc.Expression:
return boolean_expression_visit(expr, _ConvertToArrowExpression())
+def expression_to_reverted_pyarrow(expr: BooleanExpression) -> pc.Expression:
+ """Complimentary filter convertion function of expression_to_pyarrow.
Review Comment:
Could we mention that this function is used specifically for reverting an expression to pyarrow expression for the delete operation? As we discussed offline, negating a pc.Expression has the expected outcome that ignores Nulls, and only becomes a problem when we use it for deletion, because we want to avoid deleting Nulls unless it is specified in the delete_filter explicitly. On that note, I feel that it may be good to make this a private function as well. WDYT?
nit: typo
```suggestion
"""Complimentary filter conversion function of expression_to_pyarrow.
```
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,124 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _CollectNullUnmentionedTermsFromExpression(BoundBooleanExpressionVisitor[Any]):
+ def __init__(self) -> None:
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ self.is_valid_or_not_bound_terms: set[BoundTerm[Any]] = set()
+ # The remaining BoundTerms appearing in the boolean expr.
+ self.null_unmentioned_bound_terms: set[BoundTerm[Any]] = set()
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_valid_or_not_bound_terms.add(term)
+
+ def _handle_skipped(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_valid_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_skipped(term)
+
+ # todo: do I have to modify this as well
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_skipped(term)
+
+ # todo: do I have to modify this as well, might need 2 self.xx sets for mentioned_nan and none-mentioned-nan
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+
+def _get_is_valid_or_not_bound_refs(expr: BooleanExpression) -> tuple[Set[BoundReference[Any]], Set[BoundReference[Any]]]:
+ """Collect the bound terms catogorized by having at least one is_null or is_not_null in the expr and the remaining."""
Review Comment:
nit: typo
```suggestion
"""Collect the bound terms categorized by having at least one is_null or is_not_null in the expr and the remaining."""
```
##########
pyiceberg/expressions/__init__.py:
##########
@@ -135,6 +135,10 @@ def __repr__(self) -> str:
def ref(self) -> BoundReference[L]:
return self
+ def __hash__(self) -> int:
+ """Return hash value of the Record class."""
Review Comment:
nit:
```suggestion
"""Return hash value of the BoundReference class."""
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "jqin61 (via GitHub)" <gi...@apache.org>.
jqin61 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1687434820
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,124 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _CollectNullUnmentionedTermsFromExpression(BoundBooleanExpressionVisitor[Any]):
+ def __init__(self) -> None:
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ self.is_valid_or_not_bound_terms: set[BoundTerm[Any]] = set()
+ # The remaining BoundTerms appearing in the boolean expr.
+ self.null_unmentioned_bound_terms: set[BoundTerm[Any]] = set()
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_valid_or_not_bound_terms.add(term)
+
+ def _handle_skipped(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_valid_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_skipped(term)
+
+ # todo: do I have to modify this as well
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_skipped(term)
+
+ # todo: do I have to modify this as well, might need 2 self.xx sets for mentioned_nan and none-mentioned-nan
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_skipped(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+
+def _get_is_valid_or_not_bound_refs(expr: BooleanExpression) -> tuple[Set[BoundReference[Any]], Set[BoundReference[Any]]]:
+ """Collect the bound terms catogorized by having at least one is_null or is_not_null in the expr and the remaining."""
+ collector = _CollectNullUnmentionedTermsFromExpression()
+ boolean_expression_visit(expr, collector)
+ null_unmentioned_bound_terms = collector.null_unmentioned_bound_terms
+ is_valid_or_not_bound_terms = collector.is_valid_or_not_bound_terms
+
+ null_unmentioned_bound_refs: Set[BoundReference[Any]] = set()
+ is_valid_or_not_bound_refs: Set[BoundReference[Any]] = set()
+ for t in null_unmentioned_bound_terms:
+ if not isinstance(t, BoundReference):
+ raise ValueError("Collected Bound Term that is not reference.")
+ else:
+ null_unmentioned_bound_refs.add(t)
+ for t in is_valid_or_not_bound_terms:
+ if not isinstance(t, BoundReference):
+ raise ValueError("Collected Bound Term that is not reference.")
+ else:
+ is_valid_or_not_bound_refs.add(t)
+ return null_unmentioned_bound_refs, is_valid_or_not_bound_refs
+
+
def expression_to_pyarrow(expr: BooleanExpression) -> pc.Expression:
return boolean_expression_visit(expr, _ConvertToArrowExpression())
+def expression_to_reverted_pyarrow(expr: BooleanExpression) -> pc.Expression:
+ """Complimentary filter convertion function of expression_to_pyarrow.
Review Comment:
I think your suggestion totally makes sense but I am on the fence. I think the semantics is a little broader and might be useful to user? because we have 1.raw expr, then 2.'revert' expression (by ~pc_expr or by expression_to_pyarrow(Not(iceberg_expr))) and then 3.this expr which is 'comlimentary'. what do you think if we change the name from reverted to complimentary?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "jqin61 (via GitHub)" <gi...@apache.org>.
jqin61 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1688686566
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,165 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _NullNaNUnmentionedTermsCollector(BoundBooleanExpressionVisitor[Any]):
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ is_null_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ null_unmentioned_bound_terms: set[BoundTerm[Any]]
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ is_nan_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ nan_unmentioned_bound_terms: set[BoundTerm[Any]]
+
+ def __init__(self) -> None:
+ self.is_null_or_not_bound_terms = set()
+ self.null_unmentioned_bound_terms = set()
+ self.is_nan_or_not_bound_terms = set()
+ self.nan_unmentioned_bound_terms = set()
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def collect(
+ self,
+ expr: BooleanExpression,
+ ) -> None:
+ """Collect the bound references categorized by having at least one is_null or is_not_null in the expr and the remaining."""
+ boolean_expression_visit(expr, self)
+
+
def expression_to_pyarrow(expr: BooleanExpression) -> pc.Expression:
return boolean_expression_visit(expr, _ConvertToArrowExpression())
+def _expression_to_complementary_pyarrow(expr: BooleanExpression) -> pc.Expression:
+ """Complementary filter conversion function of expression_to_pyarrow.
+
+ Could not use expression_to_pyarrow(Not(expr)) to achieve this complementary effect because ~ in pyarrow.compute.Expression does not handle null.
+ """
+ collector = _NullNaNUnmentionedTermsCollector()
+ collector.collect(expr)
+
+ def _downcast_term_to_reference(bound_terms: Set[BoundTerm[Any]]) -> Set[BoundReference[Any]]:
Review Comment:
Hi I want to understand more about the difference between BoundReference and BoundTerm.
Because class _NullNaNUnmentionedTermsCollector is inheriting from BoundBooleanExpressionVisitor[Any], the visit functions have to deal with BoundTerm. And in order to get the field reference to build IsNull and IsNaN, we need to do this ugly downcasting.
I saw BoundReference is the only child class of BoundTerm so far.
What should be the best practice here? Should I create new abstract class of visitor that works with reference? Or a simpler change could be that I move the typecheck and downcasting into _handle_explicit_is_null_or_not() and its 3 siblings to make _NullNaNUnmentionedTermsCollector to have set[BoundReference[Any]] as collected results rather than set[BoundTerm[Any]]?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1690505567
##########
pyiceberg/io/pyarrow.py:
##########
@@ -576,11 +572,11 @@ def _convert_scalar(value: Any, iceberg_type: IcebergType) -> pa.scalar:
class _ConvertToArrowExpression(BoundBooleanExpressionVisitor[pc.Expression]):
- def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> pc.Expression:
+ def visit_in(self, term: BoundTerm[L], literals: Set[L]) -> pc.Expression:
Review Comment:
Thanks for spotting this. I think these are just missed from the last major typing fix of visitors:
- https://github.com/apache/iceberg/pull/6258#discussion_r1034007197
- https://github.com/apache/iceberg/pull/6308/files#diff-49144c27eab7e03926b0310aa8b513dbeb9c8d2a0d33bacf8dedbd88b4680aac
Given that this is in a concrete class, how about we just use `Any` to indicate that we accept general terms?
```suggestion
def visit_in(self, term: BoundTerm[Any], literals: Set[Any]) -> pc.Expression:
```
which is also consistent with the type in other methods.
##########
pyiceberg/io/pyarrow.py:
##########
@@ -576,11 +572,11 @@ def _convert_scalar(value: Any, iceberg_type: IcebergType) -> pa.scalar:
class _ConvertToArrowExpression(BoundBooleanExpressionVisitor[pc.Expression]):
- def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> pc.Expression:
+ def visit_in(self, term: BoundTerm[L], literals: Set[L]) -> pc.Expression:
Review Comment:
Thanks for spotting this. I think these are just missed from the last major typing fix for the visitor:
- https://github.com/apache/iceberg/pull/6258#discussion_r1034007197
- https://github.com/apache/iceberg/pull/6308/files#diff-49144c27eab7e03926b0310aa8b513dbeb9c8d2a0d33bacf8dedbd88b4680aac
Given that this is in a concrete class, how about we just use `Any` to indicate that we accept general terms?
```suggestion
def visit_in(self, term: BoundTerm[Any], literals: Set[Any]) -> pc.Expression:
```
which is also consistent with the type in other methods.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "jqin61 (via GitHub)" <gi...@apache.org>.
jqin61 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1688678348
##########
tests/io/test_pyarrow_visitor.py:
##########
@@ -580,3 +594,114 @@ 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_str() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(1, "field_str_unmentioned", StringType(), required=False), accessor=Accessor(position=0, inner=None)
+ )
+
+
+@pytest.fixture
+def bound_reference_float() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(2, "field_float_mentioned_nan", FloatType(), required=False), accessor=Accessor(position=1, inner=None)
+ )
+
+
+@pytest.fixture
+def bound_reference_double() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(3, "field_double_mentioned_null", DoubleType(), required=False),
+ accessor=Accessor(position=2, inner=None),
+ )
+
+
+@pytest.fixture
+def bound_eq_str_field(bound_reference_str: BoundReference[Any]) -> BoundEqualTo[Any]:
+ return BoundEqualTo(term=bound_reference_str, literal=literal("hello"))
+
+
+@pytest.fixture
+def bound_greater_than_float_field(bound_reference_float: BoundReference[Any]) -> BoundGreaterThan[Any]:
+ return BoundGreaterThan(term=bound_reference_float, literal=literal(100))
+
+
+@pytest.fixture
+def bound_is_nan_float_field(bound_reference_float: BoundReference[Any]) -> BoundIsNaN[Any]:
+ return BoundIsNaN(bound_reference_float)
+
+
+@pytest.fixture
+def bound_eq_double_field(bound_reference_double: BoundReference[Any]) -> BoundEqualTo[Any]:
+ return BoundEqualTo(term=bound_reference_double, literal=literal(False))
+
+
+@pytest.fixture
+def bound_is_null_double_field(bound_reference_double: BoundReference[Any]) -> BoundIsNull[Any]:
+ return BoundIsNull(bound_reference_double)
+
+
+def test_collect_null_nan_unmentioned_terms(
+ bound_eq_str_field: BoundEqualTo[Any], bound_is_nan_float_field: BoundIsNaN[Any], bound_is_null_double_field: BoundIsNull[Any]
+) -> None:
+ bound_expr = And(
+ Or(And(bound_eq_str_field, bound_is_nan_float_field), bound_is_null_double_field), Not(bound_is_nan_float_field)
+ )
+ categorized_terms = _get_null_nan_refs(bound_expr)
+
+ assert {f.field.name for f in categorized_terms[0]} == {"field_float_mentioned_nan", "field_str_unmentioned"}
+ assert {f.field.name for f in categorized_terms[1]} == {"field_str_unmentioned", "field_double_mentioned_null"}
+ assert {f.field.name for f in categorized_terms[2]} == {
+ "field_double_mentioned_null",
+ }
+ assert {f.field.name for f in categorized_terms[3]} == {"field_float_mentioned_nan"}
+
+
+def test_collect_null_nan_unmentioned_terms_with_multiple_predicates_on_the_same_term(
+ bound_eq_str_field: BoundEqualTo[Any],
+ bound_greater_than_float_field: BoundGreaterThan[Any],
+ bound_is_nan_float_field: BoundIsNaN[Any],
+ bound_eq_double_field: BoundEqualTo[Any],
+ bound_is_null_double_field: BoundIsNull[Any],
+) -> None:
+ """Test a single term appears multiple places in the expression tree"""
+ bound_expr = And(
+ Or(
+ And(bound_eq_str_field, bound_greater_than_float_field),
+ And(bound_is_nan_float_field, bound_eq_double_field),
+ bound_greater_than_float_field,
+ ),
+ Not(bound_is_null_double_field),
+ )
+ categorized_terms = _get_null_nan_refs(bound_expr)
+ assert {f.field.name for f in categorized_terms[0]} == {"field_float_mentioned_nan", "field_str_unmentioned"}
+ assert {f.field.name for f in categorized_terms[1]} == {"field_str_unmentioned", "field_double_mentioned_null"}
+ assert {f.field.name for f in categorized_terms[2]} == {
+ "field_double_mentioned_null",
+ }
+ assert {f.field.name for f in categorized_terms[3]} == {"field_float_mentioned_nan"}
+
+
+def test__expression_to_complementary_pyarrow(
+ bound_eq_str_field: BoundEqualTo[Any],
+ bound_greater_than_float_field: BoundGreaterThan[Any],
+ bound_is_nan_float_field: BoundIsNaN[Any],
+ bound_eq_double_field: BoundEqualTo[Any],
+ bound_is_null_double_field: BoundIsNull[Any],
+) -> None:
+ bound_expr = And(
+ Or(
+ And(bound_eq_str_field, bound_greater_than_float_field),
+ And(bound_is_nan_float_field, bound_eq_double_field),
+ bound_greater_than_float_field,
+ ),
+ Not(bound_is_null_double_field),
+ )
+ result = _expression_to_complementary_pyarrow(bound_expr)
+ # Notice an isNan predicate on a str column is automatically converted to always false and removed from Or and thus will not appear in the pc.expr.
+ assert (
+ repr(result)
+ == """<pyarrow.compute.Expression (((invert((((((field_str_unmentioned == "hello") and (field_float_mentioned_nan > 100)) or (is_nan(field_float_mentioned_nan) and (field_double_mentioned_null == 0))) or (field_float_mentioned_nan > 100)) and invert(is_null(field_double_mentioned_null, {nan_is_null=false})))) or is_null(field_float_mentioned_nan, {nan_is_null=false})) or is_null(field_str_unmentioned, {nan_is_null=false})) or is_nan(field_double_mentioned_null))>"""
Review Comment:
gotcha, will change it.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1687991934
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,160 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _CollectNullNaNUnmentionedTermsFromExpression(BoundBooleanExpressionVisitor[Any]):
+ def __init__(self) -> None:
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ self.is_null_or_not_bound_terms: set[BoundTerm[Any]] = set()
+ # The remaining BoundTerms appearing in the boolean expr.
+ self.null_unmentioned_bound_terms: set[BoundTerm[Any]] = set()
+
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ self.is_nan_or_not_bound_terms: set[BoundTerm[Any]] = set()
+ # The remaining BoundTerms appearing in the boolean expr.
+ self.nan_unmentioned_bound_terms: set[BoundTerm[Any]] = set()
+
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+
+def _get_null_nan_refs(
+ expr: BooleanExpression,
+) -> tuple[Set[BoundReference[Any]], Set[BoundReference[Any]], Set[BoundReference[Any]], Set[BoundReference[Any]]]:
+ """Collect the bound terms categorized by having at least one is_null or is_not_null in the expr and the remaining."""
+ collector = _CollectNullNaNUnmentionedTermsFromExpression()
+ boolean_expression_visit(expr, collector)
+
+ def _downcast_term_to_reference(bound_terms: Set[BoundTerm[Any]]) -> Set[BoundReference[Any]]:
+ """Handle mypy check for BoundTerm -> BoundReference."""
+ bound_refs: Set[BoundReference[Any]] = set()
+ for t in bound_terms:
+ if not isinstance(t, BoundReference):
+ raise ValueError("Collected Bound Term that is not reference.")
+ else:
+ bound_refs.add(t)
+ return bound_refs
+
+ null_unmentioned_bound_refs: Set[BoundReference[Any]] = _downcast_term_to_reference(collector.null_unmentioned_bound_terms)
+ is_null_or_not_bound_refs: Set[BoundReference[Any]] = _downcast_term_to_reference(collector.is_null_or_not_bound_terms)
+ nan_unmentioned_bound_refs: Set[BoundReference[Any]] = _downcast_term_to_reference(collector.nan_unmentioned_bound_terms)
+ is_nan_or_not_bound_refs: Set[BoundReference[Any]] = _downcast_term_to_reference(collector.is_nan_or_not_bound_terms)
+
+ return null_unmentioned_bound_refs, nan_unmentioned_bound_refs, is_null_or_not_bound_refs, is_nan_or_not_bound_refs
+
+
def expression_to_pyarrow(expr: BooleanExpression) -> pc.Expression:
return boolean_expression_visit(expr, _ConvertToArrowExpression())
+def _expression_to_complimentary_pyarrow(expr: BooleanExpression) -> pc.Expression:
+ """Complimentary filter conversion function of expression_to_pyarrow.
+
+ Could not use expression_to_pyarrow(Not(expr)) to achieve this complimentary effect because ~ in pc.Expression does not handle null.
Review Comment:
nit:
```suggestion
def _expression_to_complementary_pyarrow(expr: BooleanExpression) -> pc.Expression:
"""Complementary filter conversion function of expression_to_pyarrow.
Could not use expression_to_pyarrow(Not(expr)) to achieve this complementary effect because ~ in pc.Expression does not handle null.
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "jqin61 (via GitHub)" <gi...@apache.org>.
jqin61 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1688410579
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,160 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _CollectNullNaNUnmentionedTermsFromExpression(BoundBooleanExpressionVisitor[Any]):
+ def __init__(self) -> None:
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ self.is_null_or_not_bound_terms: set[BoundTerm[Any]] = set()
+ # The remaining BoundTerms appearing in the boolean expr.
+ self.null_unmentioned_bound_terms: set[BoundTerm[Any]] = set()
+
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ self.is_nan_or_not_bound_terms: set[BoundTerm[Any]] = set()
+ # The remaining BoundTerms appearing in the boolean expr.
+ self.nan_unmentioned_bound_terms: set[BoundTerm[Any]] = set()
+
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+
+def _get_null_nan_refs(
+ expr: BooleanExpression,
+) -> tuple[Set[BoundReference[Any]], Set[BoundReference[Any]], Set[BoundReference[Any]], Set[BoundReference[Any]]]:
Review Comment:
Yes I think a class is much better than these magic numbers of 0,1,2,3
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "jqin61 (via GitHub)" <gi...@apache.org>.
jqin61 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1690301032
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,152 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _NullNaNUnmentionedTermsCollector(BoundBooleanExpressionVisitor[Any]):
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ is_null_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ null_unmentioned_bound_terms: set[BoundTerm[Any]]
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ is_nan_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ nan_unmentioned_bound_terms: set[BoundTerm[Any]]
+
+ def __init__(self) -> None:
+ self.is_null_or_not_bound_terms = set()
+ self.null_unmentioned_bound_terms = set()
+ self.is_nan_or_not_bound_terms = set()
+ self.nan_unmentioned_bound_terms = set()
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
Review Comment:
thanks! this is such a good catch!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#issuecomment-2243845630
Proposed implementation is consistent with Spark Iceberg's behavior.
For a given Iceberg table:
```
spark.read.table("demo.tacocat.test_null").show()
>> +----+----+
>> | id|data|
>> +----+----+
>> | 1| a|
>> | 2| b|
>> | 3| c|
>> |NULL| d|
>> | 5| e|
>> +----+----+
```
Scan API ignore Null values unless null is specified in the predicate expression:
```
spark.sql("""SELECT * FROM demo.tacocat.test_null WHERE id > 2""").show()
>> +---+----+
>> | id|data|
>> +---+----+
>> | 1| a|
>> | 2| b|
>> +---+----+
spark.sql("""SELECT * FROM demo.tacocat.test_null WHERE not id > 2""").show()
>> +---+----+
>> | id|data|
>> +---+----+
>> | 3| c|
>> | 5| e|
>> +---+----+
spark.sql("""SELECT * FROM demo.tacocat.test_null WHERE id > 2 OR id is NULL""").show()
>> +----+----+
>> | id|data|
>> +----+----+
>> | 3| c|
>> |NULL| d|
>> | 5| e|
>> +----+----+
```
While the DELETE API avoids deleting nulls unless it is specified directly in the predicate expression.
```
spark.sql("""DELETE FROM demo.tacocat.test_null WHERE id == 2""")
spark.read.table("demo.tacocat.test_null").show()
>> +----+----+
>> | id|data|
>> +----+----+
>> | 1| a|
>> | 3| c|
>> |NULL| d|
>> | 5| e|
>> +----+----+
spark.sql("""DELETE FROM demo.tacocat.test_null WHERE id <= 2""")
spark.read.table("demo.tacocat.test_null").show()
>> +----+----+
>> | id|data|
>> +----+----+
>> | 3| c|
>> |NULL| d|
>> | 5| e|
>> +----+----+
spark.sql("""DELETE FROM demo.tacocat.test_null WHERE id <= 2 or id IS NULL""")
spark.read.table("demo.tacocat.test_null").show()
>> +---+----+
>> | id|data|
>> +---+----+
>> | 3| c|
>> | 5| e|
>> +---+----+
```
So I agree with @jqin61 's finding that we have to walk through the predicate expression to check if Nulls/NaNs were directly mentioned on delete to revert the expression correctly as proposed. Simple negation of `pyarrow.compute.Expression` will unfortunately yield the wrong outcome.
```
import pyarrow as pa
import pyarrow.compute as pc
expr = (pc.field("a") == pc.scalar(3))
tbl = pa.Table.from_pydict({"a": [1,2,3,None]})
tbl.filter(~ expr)
>> pyarrow.Table
>> a: int64
>> ----
>> a: [[1,2]]
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "jqin61 (via GitHub)" <gi...@apache.org>.
jqin61 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1689929311
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,165 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _NullNaNUnmentionedTermsCollector(BoundBooleanExpressionVisitor[Any]):
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ is_null_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ null_unmentioned_bound_terms: set[BoundTerm[Any]]
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ is_nan_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ nan_unmentioned_bound_terms: set[BoundTerm[Any]]
+
+ def __init__(self) -> None:
+ self.is_null_or_not_bound_terms = set()
+ self.null_unmentioned_bound_terms = set()
+ self.is_nan_or_not_bound_terms = set()
+ self.nan_unmentioned_bound_terms = set()
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def collect(
+ self,
+ expr: BooleanExpression,
+ ) -> None:
+ """Collect the bound references categorized by having at least one is_null or is_not_null in the expr and the remaining."""
+ boolean_expression_visit(expr, self)
+
+
def expression_to_pyarrow(expr: BooleanExpression) -> pc.Expression:
return boolean_expression_visit(expr, _ConvertToArrowExpression())
+def _expression_to_complementary_pyarrow(expr: BooleanExpression) -> pc.Expression:
+ """Complementary filter conversion function of expression_to_pyarrow.
+
+ Could not use expression_to_pyarrow(Not(expr)) to achieve this complementary effect because ~ in pyarrow.compute.Expression does not handle null.
+ """
+ collector = _NullNaNUnmentionedTermsCollector()
+ collector.collect(expr)
+
+ def _downcast_term_to_reference(bound_terms: Set[BoundTerm[Any]]) -> Set[BoundReference[Any]]:
Review Comment:
fixed the types in tests and removed downcast in [b8aa4d8](https://github.com/apache/iceberg-python/pull/955/commits/b8aa4d888aec8ef88941fd48651fc9835ce73248)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1687981851
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,160 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _CollectNullNaNUnmentionedTermsFromExpression(BoundBooleanExpressionVisitor[Any]):
+ def __init__(self) -> None:
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ self.is_null_or_not_bound_terms: set[BoundTerm[Any]] = set()
+ # The remaining BoundTerms appearing in the boolean expr.
+ self.null_unmentioned_bound_terms: set[BoundTerm[Any]] = set()
+
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ self.is_nan_or_not_bound_terms: set[BoundTerm[Any]] = set()
+ # The remaining BoundTerms appearing in the boolean expr.
+ self.nan_unmentioned_bound_terms: set[BoundTerm[Any]] = set()
+
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+
+def _get_null_nan_refs(
+ expr: BooleanExpression,
+) -> tuple[Set[BoundReference[Any]], Set[BoundReference[Any]], Set[BoundReference[Any]], Set[BoundReference[Any]]]:
Review Comment:
+1 to this suggestion, it would make the code, and tests more readable
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1687535523
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,160 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _CollectNullNaNUnmentionedTermsFromExpression(BoundBooleanExpressionVisitor[Any]):
+ def __init__(self) -> None:
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ self.is_null_or_not_bound_terms: set[BoundTerm[Any]] = set()
+ # The remaining BoundTerms appearing in the boolean expr.
+ self.null_unmentioned_bound_terms: set[BoundTerm[Any]] = set()
+
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ self.is_nan_or_not_bound_terms: set[BoundTerm[Any]] = set()
+ # The remaining BoundTerms appearing in the boolean expr.
+ self.nan_unmentioned_bound_terms: set[BoundTerm[Any]] = set()
Review Comment:
```suggestion
class _CollectNullNaNUnmentionedTermsFromExpression(BoundBooleanExpressionVisitor[Any]):
# BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
is_null_or_not_bound_terms: set[BoundTerm[Any]]
# The remaining BoundTerms appearing in the boolean expr.
null_unmentioned_bound_terms: set[BoundTerm[Any]]
# BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
is_nan_or_not_bound_terms: set[BoundTerm[Any]]
# The remaining BoundTerms appearing in the boolean expr.
nan_unmentioned_bound_terms: set[BoundTerm[Any]]
def __init__(self) -> None:
self.is_null_or_not_bound_terms = set()
self.null_unmentioned_bound_terms = set()
self.is_nan_or_not_bound_terms = set()
self.nan_unmentioned_bound_terms = set()
```
How about defining type hints above `__init__` like what we normally do in other classes?
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,160 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _CollectNullNaNUnmentionedTermsFromExpression(BoundBooleanExpressionVisitor[Any]):
+ def __init__(self) -> None:
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ self.is_null_or_not_bound_terms: set[BoundTerm[Any]] = set()
+ # The remaining BoundTerms appearing in the boolean expr.
+ self.null_unmentioned_bound_terms: set[BoundTerm[Any]] = set()
+
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ self.is_nan_or_not_bound_terms: set[BoundTerm[Any]] = set()
+ # The remaining BoundTerms appearing in the boolean expr.
+ self.nan_unmentioned_bound_terms: set[BoundTerm[Any]] = set()
+
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+
+def _get_null_nan_refs(
+ expr: BooleanExpression,
+) -> tuple[Set[BoundReference[Any]], Set[BoundReference[Any]], Set[BoundReference[Any]], Set[BoundReference[Any]]]:
Review Comment:
How about we creating a new type like [UpdatesAndRequirement](https://github.com/kevinjqliu/iceberg-python/blob/d0bfb4a67bccd15c2d2e9baa481a5f33fe4c5220/pyiceberg/table/__init__.py#L1344)?
We could also consider create a private class, for example `_NullNaNRefsCollector` to hold these sets.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1688622860
##########
tests/io/test_pyarrow_visitor.py:
##########
@@ -580,3 +594,114 @@ 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_str() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(1, "field_str_unmentioned", StringType(), required=False), accessor=Accessor(position=0, inner=None)
+ )
+
+
+@pytest.fixture
+def bound_reference_float() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(2, "field_float_mentioned_nan", FloatType(), required=False), accessor=Accessor(position=1, inner=None)
+ )
+
+
+@pytest.fixture
+def bound_reference_double() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(3, "field_double_mentioned_null", DoubleType(), required=False),
+ accessor=Accessor(position=2, inner=None),
+ )
+
+
+@pytest.fixture
+def bound_eq_str_field(bound_reference_str: BoundReference[Any]) -> BoundEqualTo[Any]:
+ return BoundEqualTo(term=bound_reference_str, literal=literal("hello"))
+
+
+@pytest.fixture
+def bound_greater_than_float_field(bound_reference_float: BoundReference[Any]) -> BoundGreaterThan[Any]:
+ return BoundGreaterThan(term=bound_reference_float, literal=literal(100))
+
+
+@pytest.fixture
+def bound_is_nan_float_field(bound_reference_float: BoundReference[Any]) -> BoundIsNaN[Any]:
+ return BoundIsNaN(bound_reference_float)
+
+
+@pytest.fixture
+def bound_eq_double_field(bound_reference_double: BoundReference[Any]) -> BoundEqualTo[Any]:
+ return BoundEqualTo(term=bound_reference_double, literal=literal(False))
+
+
+@pytest.fixture
+def bound_is_null_double_field(bound_reference_double: BoundReference[Any]) -> BoundIsNull[Any]:
+ return BoundIsNull(bound_reference_double)
+
+
+def test_collect_null_nan_unmentioned_terms(
+ bound_eq_str_field: BoundEqualTo[Any], bound_is_nan_float_field: BoundIsNaN[Any], bound_is_null_double_field: BoundIsNull[Any]
+) -> None:
+ bound_expr = And(
+ Or(And(bound_eq_str_field, bound_is_nan_float_field), bound_is_null_double_field), Not(bound_is_nan_float_field)
+ )
+ categorized_terms = _get_null_nan_refs(bound_expr)
+
+ assert {f.field.name for f in categorized_terms[0]} == {"field_float_mentioned_nan", "field_str_unmentioned"}
+ assert {f.field.name for f in categorized_terms[1]} == {"field_str_unmentioned", "field_double_mentioned_null"}
+ assert {f.field.name for f in categorized_terms[2]} == {
+ "field_double_mentioned_null",
+ }
+ assert {f.field.name for f in categorized_terms[3]} == {"field_float_mentioned_nan"}
+
+
+def test_collect_null_nan_unmentioned_terms_with_multiple_predicates_on_the_same_term(
+ bound_eq_str_field: BoundEqualTo[Any],
+ bound_greater_than_float_field: BoundGreaterThan[Any],
+ bound_is_nan_float_field: BoundIsNaN[Any],
+ bound_eq_double_field: BoundEqualTo[Any],
+ bound_is_null_double_field: BoundIsNull[Any],
+) -> None:
+ """Test a single term appears multiple places in the expression tree"""
+ bound_expr = And(
+ Or(
+ And(bound_eq_str_field, bound_greater_than_float_field),
+ And(bound_is_nan_float_field, bound_eq_double_field),
+ bound_greater_than_float_field,
+ ),
+ Not(bound_is_null_double_field),
+ )
+ categorized_terms = _get_null_nan_refs(bound_expr)
+ assert {f.field.name for f in categorized_terms[0]} == {"field_float_mentioned_nan", "field_str_unmentioned"}
+ assert {f.field.name for f in categorized_terms[1]} == {"field_str_unmentioned", "field_double_mentioned_null"}
+ assert {f.field.name for f in categorized_terms[2]} == {
+ "field_double_mentioned_null",
+ }
+ assert {f.field.name for f in categorized_terms[3]} == {"field_float_mentioned_nan"}
+
+
+def test__expression_to_complementary_pyarrow(
+ bound_eq_str_field: BoundEqualTo[Any],
+ bound_greater_than_float_field: BoundGreaterThan[Any],
+ bound_is_nan_float_field: BoundIsNaN[Any],
+ bound_eq_double_field: BoundEqualTo[Any],
+ bound_is_null_double_field: BoundIsNull[Any],
+) -> None:
+ bound_expr = And(
+ Or(
+ And(bound_eq_str_field, bound_greater_than_float_field),
+ And(bound_is_nan_float_field, bound_eq_double_field),
+ bound_greater_than_float_field,
+ ),
+ Not(bound_is_null_double_field),
+ )
+ result = _expression_to_complementary_pyarrow(bound_expr)
+ # Notice an isNan predicate on a str column is automatically converted to always false and removed from Or and thus will not appear in the pc.expr.
+ assert (
+ repr(result)
+ == """<pyarrow.compute.Expression (((invert((((((field_str_unmentioned == "hello") and (field_float_mentioned_nan > 100)) or (is_nan(field_float_mentioned_nan) and (field_double_mentioned_null == 0))) or (field_float_mentioned_nan > 100)) and invert(is_null(field_double_mentioned_null, {nan_is_null=false})))) or is_null(field_float_mentioned_nan, {nan_is_null=false})) or is_null(field_str_unmentioned, {nan_is_null=false})) or is_nan(field_double_mentioned_null))>"""
Review Comment:
Hi @jqin61 - this looks almost ready to merge. Thank you again for all your effort in fixing this critical issue in this feature for this release. Personally, I'm finding these field names difficult to understand.
I feel as though we may have named the fields this way for convenience in understanding the test case, but I'm actually finding it more difficult to understand because I can get over the fact that a field name should be agnostic of the predicate expression that is applied on it - is_nan or a predicate without nan or null can be applied on the same field.
I'm curious to source other reviewers' opinions on this as well!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "jqin61 (via GitHub)" <gi...@apache.org>.
jqin61 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1690550621
##########
pyiceberg/io/pyarrow.py:
##########
@@ -576,11 +572,11 @@ def _convert_scalar(value: Any, iceberg_type: IcebergType) -> pa.scalar:
class _ConvertToArrowExpression(BoundBooleanExpressionVisitor[pc.Expression]):
- def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> pc.Expression:
+ def visit_in(self, term: BoundTerm[L], literals: Set[L]) -> pc.Expression:
Review Comment:
sounds good!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1688971266
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,165 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _NullNaNUnmentionedTermsCollector(BoundBooleanExpressionVisitor[Any]):
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ is_null_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ null_unmentioned_bound_terms: set[BoundTerm[Any]]
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ is_nan_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ nan_unmentioned_bound_terms: set[BoundTerm[Any]]
+
+ def __init__(self) -> None:
+ self.is_null_or_not_bound_terms = set()
+ self.null_unmentioned_bound_terms = set()
+ self.is_nan_or_not_bound_terms = set()
+ self.nan_unmentioned_bound_terms = set()
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def collect(
+ self,
+ expr: BooleanExpression,
+ ) -> None:
+ """Collect the bound references categorized by having at least one is_null or is_not_null in the expr and the remaining."""
+ boolean_expression_visit(expr, self)
+
+
def expression_to_pyarrow(expr: BooleanExpression) -> pc.Expression:
return boolean_expression_visit(expr, _ConvertToArrowExpression())
+def _expression_to_complementary_pyarrow(expr: BooleanExpression) -> pc.Expression:
+ """Complementary filter conversion function of expression_to_pyarrow.
+
+ Could not use expression_to_pyarrow(Not(expr)) to achieve this complementary effect because ~ in pyarrow.compute.Expression does not handle null.
+ """
+ collector = _NullNaNUnmentionedTermsCollector()
+ collector.collect(expr)
+
+ def _downcast_term_to_reference(bound_terms: Set[BoundTerm[Any]]) -> Set[BoundReference[Any]]:
Review Comment:
Hi Adrian, I don't have much knowledge regarding the difference between a Reference and a Term and why we organized the classes this way, but I took a stab at resolving this issue without requiring a new Visitor. I've created a PR to your branch - please have a look and let me know if this works for you!
https://github.com/jqin61/iceberg-python/pull/5
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "syun64 (via GitHub)" <gi...@apache.org>.
syun64 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1688973596
##########
tests/io/test_pyarrow_visitor.py:
##########
@@ -580,3 +594,127 @@ 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_str() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(1, "string_field", StringType(), required=False), accessor=Accessor(position=0, inner=None)
+ )
+
+
+@pytest.fixture
+def bound_reference_float() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(2, "float_field", FloatType(), required=False), accessor=Accessor(position=1, inner=None)
+ )
+
+
+@pytest.fixture
+def bound_reference_double() -> BoundReference[Any]:
+ return BoundReference(
+ field=NestedField(3, "double_field", DoubleType(), required=False),
+ accessor=Accessor(position=2, inner=None),
+ )
+
+
+@pytest.fixture
+def bound_eq_str_field(bound_reference_str: BoundReference[Any]) -> BoundEqualTo[Any]:
+ return BoundEqualTo(term=bound_reference_str, literal=literal("hello"))
+
+
+@pytest.fixture
+def bound_greater_than_float_field(bound_reference_float: BoundReference[Any]) -> BoundGreaterThan[Any]:
+ return BoundGreaterThan(term=bound_reference_float, literal=literal(100))
+
+
+@pytest.fixture
+def bound_is_nan_float_field(bound_reference_float: BoundReference[Any]) -> BoundIsNaN[Any]:
+ return BoundIsNaN(bound_reference_float)
+
+
+@pytest.fixture
+def bound_eq_double_field(bound_reference_double: BoundReference[Any]) -> BoundEqualTo[Any]:
+ return BoundEqualTo(term=bound_reference_double, literal=literal(False))
+
+
+@pytest.fixture
+def bound_is_null_double_field(bound_reference_double: BoundReference[Any]) -> BoundIsNull[Any]:
+ return BoundIsNull(bound_reference_double)
+
+
+def test_collect_null_nan_unmentioned_terms(
+ bound_eq_str_field: BoundEqualTo[Any], bound_is_nan_float_field: BoundIsNaN[Any], bound_is_null_double_field: BoundIsNull[Any]
+) -> None:
+ bound_expr = And(
+ Or(And(bound_eq_str_field, bound_is_nan_float_field), bound_is_null_double_field), Not(bound_is_nan_float_field)
+ )
+ collector = _NullNaNUnmentionedTermsCollector()
+ collector.collect(bound_expr)
+ assert {f.field.name for f in collector.null_unmentioned_bound_terms} == { # type: ignore
+ "float_field",
+ "string_field",
+ }
+ assert {f.field.name for f in collector.nan_unmentioned_bound_terms} == { # type: ignore
+ "string_field",
+ "double_field",
+ }
+ assert {f.field.name for f in collector.is_null_or_not_bound_terms} == { # type: ignore
+ "double_field",
+ }
+ assert {f.field.name for f in collector.is_nan_or_not_bound_terms} == {"float_field"} # type: ignore
+
+
+def test_collect_null_nan_unmentioned_terms_with_multiple_predicates_on_the_same_term(
+ bound_eq_str_field: BoundEqualTo[Any],
+ bound_greater_than_float_field: BoundGreaterThan[Any],
+ bound_is_nan_float_field: BoundIsNaN[Any],
+ bound_eq_double_field: BoundEqualTo[Any],
+ bound_is_null_double_field: BoundIsNull[Any],
+) -> None:
+ """Test a single term appears multiple places in the expression tree"""
+ bound_expr = And(
+ Or(
+ And(bound_eq_str_field, bound_greater_than_float_field),
+ And(bound_is_nan_float_field, bound_eq_double_field),
+ bound_greater_than_float_field,
+ ),
+ Not(bound_is_null_double_field),
+ )
+ collector = _NullNaNUnmentionedTermsCollector()
+ collector.collect(bound_expr)
+ assert {f.field.name for f in collector.null_unmentioned_bound_terms} == { # type: ignore
+ "float_field",
+ "string_field",
+ }
+ assert {f.field.name for f in collector.nan_unmentioned_bound_terms} == { # type: ignore
+ "string_field",
+ "double_field",
+ }
+ assert {f.field.name for f in collector.is_null_or_not_bound_terms} == { # type: ignore
+ "double_field",
+ }
+ assert {f.field.name for f in collector.is_nan_or_not_bound_terms} == {"float_field"} # type: ignore
+
+
+def test__expression_to_complementary_pyarrow(
Review Comment:
nit:
```suggestion
def test_expression_to_complementary_pyarrow(
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix table.delete()/overwrite() with null values [iceberg-python]
Posted by "jqin61 (via GitHub)" <gi...@apache.org>.
jqin61 commented on code in PR #955:
URL: https://github.com/apache/iceberg-python/pull/955#discussion_r1688686566
##########
pyiceberg/io/pyarrow.py:
##########
@@ -638,10 +634,165 @@ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> p
return left_result | right_result
+class _NullNaNUnmentionedTermsCollector(BoundBooleanExpressionVisitor[Any]):
+ # BoundTerms which have either is_null or is_not_null appearing at least once in the boolean expr.
+ is_null_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ null_unmentioned_bound_terms: set[BoundTerm[Any]]
+ # BoundTerms which have either is_nan or is_not_nan appearing at least once in the boolean expr.
+ is_nan_or_not_bound_terms: set[BoundTerm[Any]]
+ # The remaining BoundTerms appearing in the boolean expr.
+ nan_unmentioned_bound_terms: set[BoundTerm[Any]]
+
+ def __init__(self) -> None:
+ self.is_null_or_not_bound_terms = set()
+ self.null_unmentioned_bound_terms = set()
+ self.is_nan_or_not_bound_terms = set()
+ self.nan_unmentioned_bound_terms = set()
+ super().__init__()
+
+ def _handle_explicit_is_null_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_null or is_not_null is included."""
+ if term in self.null_unmentioned_bound_terms:
+ self.null_unmentioned_bound_terms.remove(term)
+ self.is_null_or_not_bound_terms.add(term)
+
+ def _handle_null_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_null or is_not_null is included."""
+ if term not in self.is_null_or_not_bound_terms:
+ self.null_unmentioned_bound_terms.add(term)
+
+ def _handle_explicit_is_nan_or_not(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where either is_nan or is_not_nan is included."""
+ if term in self.nan_unmentioned_bound_terms:
+ self.nan_unmentioned_bound_terms.remove(term)
+ self.is_nan_or_not_bound_terms.add(term)
+
+ def _handle_nan_unmentioned(self, term: BoundTerm[Any]) -> None:
+ """Handle the predicate case where neither is_nan or is_not_nan is included."""
+ if term not in self.is_nan_or_not_bound_terms:
+ self.nan_unmentioned_bound_terms.add(term)
+
+ def visit_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_in(self, term: BoundTerm[pc.Expression], literals: Set[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_is_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_not_nan(self, term: BoundTerm[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_explicit_is_nan_or_not(term)
+
+ def visit_is_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_null(self, term: BoundTerm[Any]) -> None:
+ self._handle_explicit_is_null_or_not(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_greater_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_less_than_or_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_not_starts_with(self, term: BoundTerm[Any], literal: Literal[Any]) -> None:
+ self._handle_null_unmentioned(term)
+ self._handle_nan_unmentioned(term)
+
+ def visit_true(self) -> None:
+ return
+
+ def visit_false(self) -> None:
+ return
+
+ def visit_not(self, child_result: pc.Expression) -> None:
+ return
+
+ def visit_and(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def visit_or(self, left_result: pc.Expression, right_result: pc.Expression) -> None:
+ return
+
+ def collect(
+ self,
+ expr: BooleanExpression,
+ ) -> None:
+ """Collect the bound references categorized by having at least one is_null or is_not_null in the expr and the remaining."""
+ boolean_expression_visit(expr, self)
+
+
def expression_to_pyarrow(expr: BooleanExpression) -> pc.Expression:
return boolean_expression_visit(expr, _ConvertToArrowExpression())
+def _expression_to_complementary_pyarrow(expr: BooleanExpression) -> pc.Expression:
+ """Complementary filter conversion function of expression_to_pyarrow.
+
+ Could not use expression_to_pyarrow(Not(expr)) to achieve this complementary effect because ~ in pyarrow.compute.Expression does not handle null.
+ """
+ collector = _NullNaNUnmentionedTermsCollector()
+ collector.collect(expr)
+
+ def _downcast_term_to_reference(bound_terms: Set[BoundTerm[Any]]) -> Set[BoundReference[Any]]:
Review Comment:
Hi I want to understand more about the difference between BoundReference and BoundTerm.
Because class _NullNaNUnmentionedTermsCollector is inheriting from BoundBooleanExpressionVisitor[Any], the visit functions have to deal with BoundTerm. And in order to get the field reference to build IsNull and IsNaN, we need to do this ugly downcasting.
I saw BoundReference is the only child class of BoundTerm so far.
What should be the best practice here? Should I create new abstract class of visitor that works with reference? Or a simpler change could be that I move the typecheck and downcasting into _handle_explicit_is_null_or_not() and its 3 siblings to make _NullNaNUnmentionedTermsCollector to have set[BoundReference[Any]] as collected results rather than set[BoundTerm[Any]]?
Pls give suggestions if possible, thanks a lot! @syun64 @HonahX
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org