samredai commented on code in PR #4816:
URL: https://github.com/apache/iceberg/pull/4816#discussion_r883059232
##########
python/src/iceberg/expressions/base.py:
##########
@@ -137,6 +74,28 @@ def __invert__(self) -> "BooleanExpression":
...
+class BoundPredicate(ABC):
+ def __init__(self, left, right):
+ """A concrete predicate must have a `left` and `right`"""
+ self._left = left
+ self._right = right
Review Comment:
> Or possibly left and right correspond tot the java term and literal
arguments of BoundLiteralPredicate?
Yes exactly. The plan is to not have a generic `BoundLiteralPredicate` that
uses an `Operation` enum + a switch statement. Instead we'll have a different
class for every operation, so `In(BooleanExpression, BoundPredicate)`,
`LessThan(BooleanExpression, BoundPredicate)`, `GreaterThan(BooleanExpression,
BoundPredicate)`, etc.
Since we're working with binary expression trees, I think it makes sense to
use `left` and `right` here to keep the visitor logic intuitive, even though
this deviates from the java `term` and `literal` argument names. IMO the fact
that `left` is always a `term` and `right` is always a `literal` can simply be
described through typehints and the docstring. The `right` typehint in
particular would be even more granular, for example for the `In` class in this
PR there's `right: List[Literal]`
--
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]
