[GitHub] [iceberg] Fokko commented on a diff in pull request #6127: Python: Add expression evaluator

[GitBox <gi...@apache.org>]

Fokko commented on code in PR #6127:
URL: https://github.com/apache/iceberg/pull/6127#discussion_r1015073328


##########
python/pyiceberg/expressions/visitors.py:
##########
@@ -417,6 +422,75 @@ def visit_bound_predicate(self, predicate) -> BooleanExpression:
         return predicate
 
 
+def expression_evaluator(schema: Schema, unbound: BooleanExpression, case_sensitive=True) -> Callable[[StructProtocol], bool]:

Review Comment:
   ```suggestion
   def expression_evaluator(schema: Schema, unbound: BooleanExpression, case_sensitive: bool = True) -> Callable[[StructProtocol], bool]:
   ```



##########
python/pyiceberg/expressions/visitors.py:
##########
@@ -417,6 +422,75 @@ def visit_bound_predicate(self, predicate) -> BooleanExpression:
         return predicate
 
 
+def expression_evaluator(schema: Schema, unbound: BooleanExpression, case_sensitive=True) -> Callable[[StructProtocol], bool]:
+    return _ExpressionEvaluator(schema, unbound, case_sensitive).eval
+
+
+class _ExpressionEvaluator(BoundBooleanExpressionVisitor[bool]):
+    bound: BooleanExpression
+    struct: StructProtocol
+
+    def __init__(self, schema: Schema, unbound: BooleanExpression, case_sensitive=True):

Review Comment:
   ```suggestion
       def __init__(self, schema: Schema, unbound: BooleanExpression, case_sensitive: bool = True):
   ```



##########
python/pyiceberg/expressions/visitors.py:
##########
@@ -417,6 +422,75 @@ def visit_bound_predicate(self, predicate) -> BooleanExpression:
         return predicate
 
 
+def expression_evaluator(schema: Schema, unbound: BooleanExpression, case_sensitive=True) -> Callable[[StructProtocol], bool]:
+    return _ExpressionEvaluator(schema, unbound, case_sensitive).eval
+
+
+class _ExpressionEvaluator(BoundBooleanExpressionVisitor[bool]):
+    bound: BooleanExpression
+    struct: StructProtocol
+
+    def __init__(self, schema: Schema, unbound: BooleanExpression, case_sensitive=True):
+        self.bound = bind(schema, unbound, case_sensitive)
+
+    def eval(self, struct: StructProtocol):

Review Comment:
   ```suggestion
       def eval(self, struct: StructProtocol) -> bool:
   ```



##########
python/pyiceberg/expressions/__init__.py:
##########
@@ -281,6 +289,30 @@ def __invert__(self) -> BoundIsNull:
         return BoundIsNull(self.term)
 
 
+def coerce_unary_arguments(data_class: Union[type, None]) -> Union[type, Callable[[type], type]]:

Review Comment:
   I'm not sold on this, mostly because it adds another layer of complexity. These things should normally just happen in the init. I've created a PR https://github.com/apache/iceberg/pull/6127 That does exactly this, well tested 👍🏻 



##########
python/pyiceberg/expressions/__init__.py:
##########
@@ -351,17 +386,56 @@ def bind(self, schema: Schema, case_sensitive: bool = True) -> BooleanExpression
 
 
 @dataclass(frozen=True)
-class BoundSetPredicate(BoundPredicate[T]):
-    literals: set[Literal[T]]
+class BoundSetPredicate(BoundPredicate[C]):
+    literals: set[Literal[C]]
+
+    @property

Review Comment:
   ```suggestion
       @cached_property
   ```
   This way we don't have to build the caching mechanism ourselves.



-- 
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