Fokko commented on code in PR #2565:
URL: https://github.com/apache/iceberg-python/pull/2565#discussion_r2445053802
##########
pyiceberg/expressions/__init__.py:
##########
@@ -302,12 +302,19 @@ def __getnewargs__(self) -> Tuple[BooleanExpression,
BooleanExpression]:
return (self.left, self.right)
-class Or(BooleanExpression):
+class Or(IcebergBaseModel, BooleanExpression):
"""OR operation expression - logical disjunction."""
+ model_config = ConfigDict(arbitrary_types_allowed=True)
+
+ type: TypingLiteral["str"] = Field(default="or", alias="type")
left: BooleanExpression
right: BooleanExpression
+ def __init__(self, left: BooleanExpression, right: BooleanExpression,
*rest: BooleanExpression) -> None:
+ if isinstance(self, Or) and not hasattr(self, "left") and not
hasattr(self, "right"):
+ super().__init__(left=left, right=right)
+
Review Comment:
I spent several hours debugging this, and it is pretty weird. Pydantic seems
not to work well with `__new__`. I came up with the following:
```python
class Or(IcebergBaseModel, BooleanExpression):
"""OR operation expression - logical disjunction."""
- model_config = ConfigDict(arbitrary_types_allowed=True)
+ model_config = ConfigDict(arbitrary_types_allowed=True, frozen=False)
- type: TypingLiteral["str"] = Field(default="or", alias="type")
- left: BooleanExpression
- right: BooleanExpression
+ type: TypingLiteral["or"] = Field(default="or")
+ left: BooleanExpression = Field()
+ right: BooleanExpression = Field()
def __init__(self, left: BooleanExpression, right: BooleanExpression,
*rest: BooleanExpression) -> None:
- if isinstance(self, Or) and not hasattr(self, "left") and not
hasattr(self, "right"):
- super().__init__(left=left, right=right)
+ super().__init__(left=self.left, right=self.right)
def __new__(cls, left: BooleanExpression, right: BooleanExpression,
*rest: BooleanExpression) -> BooleanExpression: # type: ignore
if rest:
@@ -326,20 +325,11 @@ class Or(IcebergBaseModel, BooleanExpression):
return left
else:
obj = super().__new__(cls)
+ obj.__pydantic_fields_set__ = set()
+ obj.left = left
+ obj.right = right
return obj
```
I see what direction you were going, but if we leave out the `obj.left =
left` then we don't have the attributes and we have an empty `Or` object. The
test is still failing, but I think that's because the UnaryPredicates are not
yet serializable. If you update the tests to use LiteralPredicates, then it
should work 👍
--
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]
