Copilot commented on code in PR #3421:
URL: https://github.com/apache/iceberg-python/pull/3421#discussion_r3368049269
##########
pyiceberg/expressions/__init__.py:
##########
@@ -540,6 +540,7 @@ def __str__(self) -> str:
return f"{str(self.__class__.__name__)}(term={str(self.term)})"
def bind(self, schema: Schema, case_sensitive: bool = True) ->
BoundUnaryPredicate:
+ assert isinstance(self.term, UnboundTerm)
Review Comment:
`assert isinstance(...)` is not a reliable runtime guard in library code
because assertions are stripped with `python -O`, which can turn these into
potential `AttributeError`/undefined behavior if `self.term` is ever a `str`
(e.g., post-init mutation, deserialization edge cases). Prefer either (a)
making the field type `Annotated[UnboundTerm,
BeforeValidator(_to_unbound_term)]` (so `self.term` is always an `UnboundTerm`
after validation and the asserts can be removed), or (b) using an explicit
runtime check that raises a regular exception (e.g., `TypeError`) and/or
`typing.cast` if the goal is only to satisfy static typing.
##########
pyiceberg/expressions/__init__.py:
##########
@@ -870,7 +872,7 @@ def as_bound(self) -> type[BoundNotIn]: # type: ignore
class LiteralPredicate(UnboundPredicate, ABC):
type: TypingLiteral["lt", "lt-eq", "gt", "gt-eq", "eq", "not-eq",
"starts-with", "not-starts-with"] = Field(alias="type")
- term: UnboundTerm
+ term: Annotated[str | UnboundTerm, BeforeValidator(_to_unbound_term)]
Review Comment:
`assert isinstance(...)` is not a reliable runtime guard in library code
because assertions are stripped with `python -O`, which can turn these into
potential `AttributeError`/undefined behavior if `self.term` is ever a `str`
(e.g., post-init mutation, deserialization edge cases). Prefer either (a)
making the field type `Annotated[UnboundTerm,
BeforeValidator(_to_unbound_term)]` (so `self.term` is always an `UnboundTerm`
after validation and the asserts can be removed), or (b) using an explicit
runtime check that raises a regular exception (e.g., `TypeError`) and/or
`typing.cast` if the goal is only to satisfy static typing.
##########
pyiceberg/expressions/__init__.py:
##########
@@ -508,7 +508,7 @@ def as_unbound(self) -> type[UnboundPredicate]: ...
class UnboundPredicate(Unbound, BooleanExpression, ABC):
model_config = ConfigDict(arbitrary_types_allowed=True)
- term: UnboundTerm
+ term: Annotated[str | UnboundTerm, BeforeValidator(_to_unbound_term)]
Review Comment:
`assert isinstance(...)` is not a reliable runtime guard in library code
because assertions are stripped with `python -O`, which can turn these into
potential `AttributeError`/undefined behavior if `self.term` is ever a `str`
(e.g., post-init mutation, deserialization edge cases). Prefer either (a)
making the field type `Annotated[UnboundTerm,
BeforeValidator(_to_unbound_term)]` (so `self.term` is always an `UnboundTerm`
after validation and the asserts can be removed), or (b) using an explicit
runtime check that raises a regular exception (e.g., `TypeError`) and/or
`typing.cast` if the goal is only to satisfy static typing.
##########
pyiceberg/expressions/__init__.py:
##########
@@ -696,6 +697,7 @@ def __init__(
super().__init__(term=_to_unbound_term(term), values=literal_set)
def bind(self, schema: Schema, case_sensitive: bool = True) ->
BoundSetPredicate:
+ assert isinstance(self.term, UnboundTerm)
Review Comment:
`assert isinstance(...)` is not a reliable runtime guard in library code
because assertions are stripped with `python -O`, which can turn these into
potential `AttributeError`/undefined behavior if `self.term` is ever a `str`
(e.g., post-init mutation, deserialization edge cases). Prefer either (a)
making the field type `Annotated[UnboundTerm,
BeforeValidator(_to_unbound_term)]` (so `self.term` is always an `UnboundTerm`
after validation and the asserts can be removed), or (b) using an explicit
runtime check that raises a regular exception (e.g., `TypeError`) and/or
`typing.cast` if the goal is only to satisfy static typing.
##########
pyiceberg/expressions/__init__.py:
##########
@@ -885,6 +887,7 @@ def literal(self) -> LiteralValue:
return self.value
def bind(self, schema: Schema, case_sensitive: bool = True) ->
BoundLiteralPredicate:
+ assert isinstance(self.term, UnboundTerm)
Review Comment:
`assert isinstance(...)` is not a reliable runtime guard in library code
because assertions are stripped with `python -O`, which can turn these into
potential `AttributeError`/undefined behavior if `self.term` is ever a `str`
(e.g., post-init mutation, deserialization edge cases). Prefer either (a)
making the field type `Annotated[UnboundTerm,
BeforeValidator(_to_unbound_term)]` (so `self.term` is always an `UnboundTerm`
after validation and the asserts can be removed), or (b) using an explicit
runtime check that raises a regular exception (e.g., `TypeError`) and/or
`typing.cast` if the goal is only to satisfy static typing.
--
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]
