Fokko commented on code in PR #6139:
URL: https://github.com/apache/iceberg/pull/6139#discussion_r1022019082
##########
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 think so. Because we can use OOP now (because we've removed the data
classes), we're able to solve this using inheritance in a cleaner way. Also, we
remove all the types and return always a `BooleanExpression` after binding,
which makes us lose the type information.
Also, we're not able to pass in the `[T]`:
```python
@dataclass(frozen=True)
class GreaterThan(LiteralPredicate[T]):
as_bound = BoundGreaterThan[T]
```
Produces:
```
python/pyiceberg/expressions/__init__.py:518: error: Can't use bound type
variable "T" to define generic alias
Found 1 error in 1 file (checked 83 source files)
```
But most importantly, we get no type checking at all. On current master, if
I add some bogus arguments:
```python
@dataclass(frozen=True)
class UnaryPredicate(UnboundPredicate[T]):
def bind(self, schema: Schema, case_sensitive: bool = True) ->
BooleanExpression:
bound_term = self.term.bind(schema, case_sensitive)
return self.as_bound(bound_term, 1, 2, 3) # The int arguments are
not expected
```
It just runs fine:
```
➜ python git:(master) ✗ pre-commit run --all-files mypy
mypy.....................................................................Passed
```
If I do the same on this branch:
```
➜ python git:(fd-remove-dataclass) pre-commit run --all-files mypy
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1
python/pyiceberg/expressions/__init__.py:325: error: Too many arguments for
"BoundUnaryPredicate"
Found 1 error in 1 file (checked 83 source files)
```
This is mostly because `as_bound: ClassVar[type]`, which should be
`ClassVar[Type[Bound]]`, and add:
```python
class SetPredicate(UnboundPredicate[T]):
as_bound: ClassVar[Type[BoundSetPredicate]]
```
For the `{Unary,Literal,Set}Predicate`, but why not use inheritance for that?
Next to that, for the projection transforms we need to have it the other way
around `as_unbound`:
https://github.com/apache/iceberg/pull/6128/files#diff-6e61823ae852914cf80ce42354961efd01d99b58fa7804d931ba9cf7ba8edac8R312-R315
We can't do it the same way because it creates a circular reference. I think
it is nice to have it the same way instead of having one ClassVar and one
property. Unfortunately two ClassVars is not going to work:
https://github.com/apache/iceberg/pull/6139#discussion_r1020962085
--
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
