Fokko commented on code in PR #5258:
URL: https://github.com/apache/iceberg/pull/5258#discussion_r928739649
##########
python/pyiceberg/expressions/base.py:
##########
@@ -156,7 +158,7 @@ def bind(self, schema: Schema, case_sensitive: bool) ->
BoundReference[T]:
Returns:
BoundReference: A reference bound to the specific field in the
Iceberg schema
"""
- field = schema.find_field(name_or_id=self.name,
case_sensitive=case_sensitive)
+ field = schema.find_field(name_or_id=self.name,
case_sensitive=case_sensitive) # pylint: disable=W0621
Review Comment:
```suggestion
field = schema.find_field(name_or_id=self.name,
case_sensitive=case_sensitive) # pylint: disable=redefined-outer-name
```
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) ->
BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] |
Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else
(literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
+ raise AttributeError(f"{self.__class__.__name__} must have exactly
1 literal.")
+
+ @property
+ def term(self) -> BoundTerm[T]:
+ return self._term
+
+ @property
+ def literals(self) -> tuple[Literal[T], ...]:
+ return self._literals # type: ignore
+
+ def __str__(self) -> str:
+ return f"{self.__class__.__name__}({str(self.term)}{self.literals and
', '+str(self.literals)})"
+
+ def __repr__(self) -> str:
+ return f"{self.__class__.__name__}({repr(self.term)}{self.literals and
', '+repr(self.literals)})"
+
+ def __eq__(self, other) -> bool:
+ return id(self) == id(other) or (
+ type(self) == type(other) and self.term == other.term and
self.literals == other.literals
+ )
-@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
- term: Reference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: UnboundTerm[T], literals: tuple[Literal[T], ...]
| Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else
(literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
+ raise AttributeError(f"{self.__class__.__name__} must have exactly
1 literal.")
+
+ @property
+ def term(self) -> UnboundTerm[T]:
+ return self._term
+
+ @property
+ def literals(self) -> tuple[Literal[T], ...]:
+ return self._literals # type: ignore
+
+ def __str__(self) -> str:
+ return f"{self.__class__.__name__}({str(self.term)}{self.literals and
', '+str(self.literals)})"
+
+ def __repr__(self) -> str:
+ return f"{self.__class__.__name__}({repr(self.term)}{self.literals and
', '+repr(self.literals)})"
+
+ def __eq__(self, other) -> bool:
Review Comment:
We could also leverage the Python dataclasses and have the eq method
generated.
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) ->
BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] |
Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else
(literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
Review Comment:
```suggestion
if not self.literals or len(self.literals) != 1:
```
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) ->
BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
Review Comment:
For readability and type checking it is best to also define the variables in
the class itself:
```python
class BoundPredicate(Bound[T], BooleanExpression):
_term: BoundTerm[T]
_literals: Tuple[Literal[T]]
```
This will also remove the ignore below:
```python
@property
def literals(self) -> tuple[Literal[T], ...]:
return self._literals # type: ignore
```
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) ->
BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] |
Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else
(literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
Review Comment:
I'm a bit confused by the signature of the `__init__`, that allows us to
pass in a `None`, and also the default value is `None`, but we clearly require
the literal to be set. Wouldn't it be better to also set this in the signature?
Instead of:
```python
def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] |
Literal[T] | None = None):
```
Go for something like:
```python
def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T]] |
Literal[T]):
```
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) ->
BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] |
Literal[T] | None = None):
+ self._term = term
+ self._literals = literals if isinstance(literals, tuple) else
(literals and (literals,))
+ self._validate_literals()
+
+ def _validate_literals(self):
+ if self.literals is None or len(self.literals) != 1:
+ raise AttributeError(f"{self.__class__.__name__} must have exactly
1 literal.")
+
+ @property
+ def term(self) -> BoundTerm[T]:
+ return self._term
+
+ @property
+ def literals(self) -> tuple[Literal[T], ...]:
+ return self._literals # type: ignore
+
+ def __str__(self) -> str:
+ return f"{self.__class__.__name__}({str(self.term)}{self.literals and
', '+str(self.literals)})"
+
+ def __repr__(self) -> str:
+ return f"{self.__class__.__name__}({repr(self.term)}{self.literals and
', '+repr(self.literals)})"
+
+ def __eq__(self, other) -> bool:
+ return id(self) == id(other) or (
+ type(self) == type(other) and self.term == other.term and
self.literals == other.literals
+ )
-@dataclass(frozen=True) # type: ignore[misc]
class UnboundPredicate(Unbound[T, BooleanExpression], BooleanExpression):
Review Comment:
This one is missing the `bind()` method
##########
python/pyiceberg/expressions/base.py:
##########
@@ -169,16 +171,64 @@ def bind(self, schema: Schema, case_sensitive: bool) ->
BoundReference[T]:
return BoundReference(field=field, accessor=accessor)
-@dataclass(frozen=True) # type: ignore[misc]
class BoundPredicate(Bound[T], BooleanExpression):
- term: BoundReference[T]
- literals: Tuple[Literal[T], ...]
+ def __init__(self, term: BoundTerm[T], literals: tuple[Literal[T], ...] |
Literal[T] | None = None):
Review Comment:
What do you think of just accumulating the positional arguments into a list
of literals?
```suggestion
def __init__(self, term: BoundTerm[T], *literals: Literal[T]):
```
--
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]
