Fokko commented on code in PR #6139:
URL: https://github.com/apache/iceberg/pull/6139#discussion_r1020961770
##########
python/pyiceberg/expressions/__init__.py:
##########
@@ -90,16 +110,25 @@ def eval(self, struct: StructProtocol) -> T:
"""
return self.accessor.get(struct)
- def ref(self) -> BoundReference[T]:
+ def __eq__(self, other):
+ return self.field == other.field if isinstance(other, BoundReference)
else False
+
+ def __repr__(self) -> str:
+ return f"BoundReference(field={repr(self.field)},
accessor={repr(self.accessor)})"
+
+ def ref(self) -> BoundReference:
return self
-class UnboundTerm(Term[T], Unbound[BoundTerm[T]], ABC):
+class UnboundTerm(Term[Any], Unbound, ABC):
Review Comment:
Since we don't know the type before binding, I set this to `Any`.
`UnaryPredicate` does not carry any type. `LiteralPredicate` and `SetPredicate`
do depending on the literals that are passed in.
##########
python/pyiceberg/expressions/literals.py:
##########
@@ -108,7 +110,7 @@ def __ge__(self, other):
@singledispatch
-def literal(value) -> Literal:
+def literal(value: Any) -> Literal[T]:
Review Comment:
Unfortunately, we can't do `value: T -> Literal[T]` because a `date` is
converted into an `int`
##########
python/pyiceberg/expressions/__init__.py:
##########
@@ -48,12 +64,13 @@ class Bound(ABC):
"""Represents a bound value expression"""
-class Unbound(Generic[B], ABC):
+class Unbound(ABC):
"""Represents an unbound value expression"""
+ @property
@abstractmethod
- def bind(self, schema: Schema, case_sensitive: bool = True) -> B:
- ... # pragma: no cover
+ def as_bound(self) -> Type[Bound]:
Review Comment:
I removed the `B` type using the OOP hierarchy.
##########
python/pyiceberg/typedef.py:
##########
@@ -36,3 +38,7 @@ def update(self, *args: Any, **kwargs: Any) -> None:
Identifier = Tuple[str, ...]
Properties = Dict[str, str]
RecursiveDict = Dict[str, Union[str, "RecursiveDict"]] # type: ignore
+
+LITERAL_TYPES = Union[str, int, bool, float, bytes, Decimal, UUID]
+
+T = TypeVar("T", bound=LITERAL_TYPES)
Review Comment:
Moved `T` to typedef with all the physical types bound.
##########
python/pyiceberg/expressions/visitors.py:
##########
@@ -60,32 +61,34 @@
PrimitiveType,
)
+B = TypeVar("B")
Review Comment:
Represents a boolean expression, we reserve `T` for the physical types. We
also replace `Any` with `T` for the literals.
##########
python/pyiceberg/expressions/__init__.py:
##########
@@ -126,10 +164,13 @@ def bind(self, schema: Schema, case_sensitive: bool =
True) -> BoundReference[T]
"""
field = schema.find_field(name_or_id=self.name,
case_sensitive=case_sensitive)
accessor = schema.accessor_for_field(field.field_id)
- return BoundReference(field=field, accessor=accessor)
+ return self.as_bound(field=field, accessor=accessor)
+
+ @property
+ def as_bound(self) -> Type[BoundReference]:
Review Comment:
I moved the types as a property, rather than a `ClassVar`, this is because a
ClassVar is referenced right away:
```python
class Unbound:
as_bound: Bound # Not allowed, since it is not yet evaluated
@property
def as_bound(self) -> Type[Bound]: # this is fine because of delay
evaluation: https://peps.python.org/pep-0563/
return Bound
class Bound:
...
```
--
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
