Fokko commented on code in PR #6139:
URL: https://github.com/apache/iceberg/pull/6139#discussion_r1022532704
##########
python/pyiceberg/expressions/visitors.py:
##########
@@ -60,32 +61,34 @@
PrimitiveType,
)
+B = TypeVar("B")
Review Comment:
Let me take you through it. Where I ran into issues is here:
https://github.com/apache/iceberg/blob/157b707a5407da9361af83a9999b007b975cfdf6/python/pyiceberg/expressions/visitors.py#L317-L322
So we have `BoundPredicate[T]`, and also the visitor returns `T`. I believe
this doesn't make sense.
For example, when we look at the `_RewriteNotVisitor`:
```python
class _RewriteNotVisitor(BooleanExpressionVisitor[BooleanExpression]):
"""Inverts the negations"""
...
def visit_bound_predicate(self, predicate) -> BooleanExpression:
return predicate
```
For convenience, we just omitted all the types. If we would add them, we'll
end up with:
```python
class _RewriteNotVisitor(BooleanExpressionVisitor[BooleanExpression]):
"""Inverts the negations"""
...
def visit_bound_predicate(self, predicate:
BoundPredicate[BooleanExpression]) -> BooleanExpression:
return predicate
```
`T` is set to `BooleanExpression`, but I don't believe we support
`BoundPredicate[BooleanExpression]`. Therefore I think it is best to reserve
`T` for `Literal[T]` and `FieldTypes` in the future, and use `B` (or `R`) for
the visitors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
