rdblue commented on code in PR #6139:
URL: https://github.com/apache/iceberg/pull/6139#discussion_r1022080677


##########
python/pyiceberg/expressions/__init__.py:
##########
@@ -321,209 +398,271 @@ def __invert__(self) -> BoundIsNaN[T]:
         return BoundIsNaN(self.term)
 
 
-@dataclass(frozen=True)
-class IsNaN(UnaryPredicate[T]):
-    as_bound = BoundIsNaN
-
-    def __invert__(self) -> NotNaN[T]:
+class IsNaN(UnaryPredicate):
+    def __invert__(self) -> NotNaN:
         return NotNaN(self.term)
 
+    @property
+    def as_bound(self) -> Type[BoundIsNaN[T]]:
+        return BoundIsNaN[T]
 
-@dataclass(frozen=True)
-class NotNaN(UnaryPredicate[T]):
-    as_bound = BoundNotNaN
 
-    def __invert__(self) -> IsNaN[T]:
+class NotNaN(UnaryPredicate):
+    def __invert__(self) -> IsNaN:
         return IsNaN(self.term)
 
+    @property
+    def as_bound(self) -> Type[BoundNotNaN[T]]:
+        return BoundNotNaN[T]
+
 
-@dataclass(frozen=True)
-class SetPredicate(UnboundPredicate[T]):
-    literals: tuple[Literal[T], ...]
+class SetPredicate(Generic[T], UnboundPredicate, ABC):
+    literals: Set[Literal[T]]
 
-    def __invert__(self) -> SetPredicate[T]:
-        """Inverted expression of the SetPredicate"""
-        raise NotImplementedError
+    def __init__(self, term: Union[str, UnboundTerm], literals: 
Union[Iterable[Any], Iterable[Literal[T]]]):
+        super().__init__(term)
+        self.literals = _convert_into_set(literals)
 
-    def bind(self, schema: Schema, case_sensitive: bool = True) -> 
BooleanExpression:
+    def bind(self, schema: Schema, case_sensitive: bool = True) -> 
BoundSetPredicate:
         bound_term = self.term.bind(schema, case_sensitive)
         return self.as_bound(bound_term, 
{lit.to(bound_term.ref().field.field_type) for lit in self.literals})
 
+    def __str__(self):
+        # Sort to make it deterministic
+        return f"{str(self.__class__.__name__)}({str(self.term)}, {{{', 
'.join(sorted([str(literal) for literal in self.literals]))}}})"
+
+    def __repr__(self) -> str:
+        # Sort to make it deterministic
+        return f"{str(self.__class__.__name__)}({repr(self.term)}, {{{', 
'.join(sorted([repr(literal) for literal in self.literals]))}}})"
+
+    def __eq__(self, other: Any) -> bool:
+        return self.term == other.term and self.literals == other.literals if 
isinstance(other, SetPredicate) else False
+
+    @property
+    @abstractmethod
+    def as_bound(self) -> Type[BoundSetPredicate[T]]:
+        return BoundSetPredicate[T]
+
 
-@dataclass(frozen=True)
-class BoundSetPredicate(BoundPredicate[T]):
-    literals: set[Literal[T]]
+class BoundSetPredicate(BoundPredicate[T], ABC):
+    literals: Set[Literal[T]]
 
-    def __invert__(self) -> BoundSetPredicate[T]:
-        """Inverted expression of the SetPredicate"""
-        raise NotImplementedError
+    def __init__(self, term: BoundTerm[T], literals: Union[Iterable[Any], 
Iterable[Literal[T]]]):
+        super().__init__(term)
+        self.literals = _convert_into_set(literals)  # pylint: disable=W0621
+
+    def __str__(self):
+        # Sort to make it deterministic
+        return f"{str(self.__class__.__name__)}({str(self.term)}, {{{', 
'.join(sorted([str(literal) for literal in self.literals]))}}})"
+
+    def __repr__(self) -> str:
+        # Sort to make it deterministic
+        return f"{str(self.__class__.__name__)}({repr(self.term)}, {{{', 
'.join(sorted([repr(literal) for literal in self.literals]))}}})"
+
+    def __eq__(self, other: Any) -> bool:
+        return self.term == other.term and self.literals == other.literals if 
isinstance(other, BoundSetPredicate) else False
 
 
-@dataclass(frozen=True)
 class BoundIn(BoundSetPredicate[T]):
-    def __new__(cls, term: BoundTerm[T], literals: set[Literal[T]]):  # 
pylint: disable=W0221
-        count = len(literals)
+    def __new__(cls, term: BoundTerm[T], literals: Union[Iterable[Any], 
Iterable[Literal[T]]]):  # pylint: disable=W0221

Review Comment:
   Since this is only intended to be called inside a `bind` method, can we skip 
accepting `Iterable[Any]` and be strict that the incoming type should be 
`Iterable[Literal[T]]`? That helps keep bound expression type safety.



-- 
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

Reply via email to