samredai commented on code in PR #5303:
URL: https://github.com/apache/iceberg/pull/5303#discussion_r975772854
##########
python/tests/expressions/test_expressions_base.py:
##########
@@ -841,3 +924,290 @@ def test_not_expression_binding(unbound_not_expression,
expected_bound_expressio
"""Test that visiting an unbound NOT expression with a bind-visitor
returns the expected bound expression"""
bound_expression = base.visit(unbound_not_expression,
visitor=base.BindVisitor(schema=table_schema_simple))
assert bound_expression == expected_bound_expression
+
+
+def test_bound_boolean_expression_visitor_ignore_is_nan_for_string_type():
+ """Test ignoring IsNan expressions when the field type is a String"""
Review Comment:
I see, I noticed the behavior so included this test but what you're saying
makes sense. So is it safe to just remove this test and expect that the
higher-level APIs would prevent an `IsNaN` expression on a string type? Or do
you feel that we need to add some validation here? (sounds like we don't need
that validation but just want to confirm)
```py
@visit_bound_predicate.register(BoundIsNaN)
def _(expr: BoundIsNaN, visitor: BoundBooleanExpressionVisitor[T]) -> T:
# validate that `expr.term.field.field_type` is an appropriate type
return visitor.visit_is_nan(term=expr.term)
```
##########
python/pyiceberg/expressions/base.py:
##########
@@ -672,3 +683,166 @@ def visit_unbound_predicate(self, predicate) ->
BooleanExpression:
def visit_bound_predicate(self, predicate) -> BooleanExpression:
raise TypeError(f"Found already bound predicate: {predicate}")
+
+
+class BoundBooleanExpressionVisitor(BooleanExpressionVisitor[T], ABC):
+ @abstractmethod
+ def visit_in(self, term: BoundTerm[T], literals: set[Literal[Any]]) -> T:
+ """Visit a bound In predicate"""
+
+ @abstractmethod
+ def visit_not_in(self, term: BoundTerm[T], literals: set[Literal[Any]]) ->
T:
+ """Visit a bound NotIn predicate"""
+
+ @abstractmethod
+ def visit_is_nan(self, term: BoundTerm[T]) -> T:
+ """Visit a bound IsNan predicate"""
+
+ @abstractmethod
+ def visit_not_nan(self, term: BoundTerm[T]) -> T:
+ """Visit a bound NotNan predicate"""
+
+ @abstractmethod
+ def visit_is_null(self, term: BoundTerm[T]) -> T:
+ """Visit a bound IsNull predicate"""
+
+ @abstractmethod
+ def visit_not_null(self, term: BoundTerm[T]) -> T:
+ """Visit a bound NotNull predicate"""
+
+ @abstractmethod
+ def visit_equal(self, term: BoundTerm[T], literal: Literal[Any]) -> T:
+ """Visit a bound Equal predicate"""
+
+ @abstractmethod
+ def visit_not_equal(self, term: BoundTerm[T], literal: Literal[Any]) -> T:
+ """Visit a bound NotEqual predicate"""
+
+ @abstractmethod
+ def visit_greater_than_or_equal(self, term: BoundTerm[T], literal:
Literal[Any]) -> T:
+ """Visit a bound GreaterThanOrEqual predicate"""
+
+ @abstractmethod
+ def visit_greater_than(self, term: BoundTerm[T], literal: Literal[Any]) ->
T:
+ """Visit a bound GreaterThan predicate"""
+
+ @abstractmethod
+ def visit_less_than(self, term: BoundTerm[T], literal: Literal[Any]) -> T:
+ """Visit a bound LessThan predicate"""
+
+ @abstractmethod
+ def visit_less_than_or_equal(self, term: BoundTerm[T], literal:
Literal[Any]) -> T:
+ """Visit a bound LessThanOrEqual predicate"""
+
+ @abstractmethod
+ def visit_true(self) -> T:
+ """Visit a bound True predicate"""
+
+ @abstractmethod
+ def visit_false(self) -> T:
+ """Visit a bound False predicate"""
+
+ @abstractmethod
+ def visit_not(self, child_result: T) -> T:
+ """Visit a bound Not predicate"""
+
+ @abstractmethod
+ def visit_and(self, left_result: T, right_result: T) -> T:
+ """Visit a bound And predicate"""
+
+ @abstractmethod
+ def visit_or(self, left_result: T, right_result: T) -> T:
+ """Visit a bound Or predicate"""
+
+ def visit_unbound_predicate(self, predicate: UnboundPredicate[T]):
+ """Visit an unbound predicate
+ Args:
+ predicate (UnboundPredicate[T]): An unbound predicate
+ Raises:
+ TypeError: This always raises since an unbound predicate is not
expected in a bound boolean expression
+ """
+ raise TypeError(f"Not a bound predicate: {predicate}")
+
+ def visit_bound_predicate(self, predicate: BoundPredicate[T]) -> T:
+ """Visit a bound predicate
+ Args:
+ predicate (BoundPredicate[T]): A bound predicate
+ """
+ return visit_bound_predicate(predicate, self)
+
+
+@singledispatch
+def visit_bound_predicate(obj, visitor: BooleanExpressionVisitor[T]) -> T: #
pylint: disable=unused-argument
+ pass
+
+
+@visit_bound_predicate.register(BoundIn)
+def _(obj: BoundIn, visitor: BoundBooleanExpressionVisitor[T]) -> T:
+ """Visit a bound In predicate"""
+ return visitor.visit_in(term=obj.term, literals=obj.literals)
+
+
+@visit_bound_predicate.register(BoundNotIn)
+def _(obj: BoundNotIn, visitor: BoundBooleanExpressionVisitor[T]) -> T:
Review Comment:
Updated 👍
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
