-
Notifications
You must be signed in to change notification settings - Fork 412
Make Not expression JSON serializable #2593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
rambleraptor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write a test for:
- Converting a Not Expression to JSON
- Converting a JSON Not Expression -> Python Not Expression
This does just enough Pydantic magic that I'm concerned about potential side-effects and a test will help ground us.
ee4fac1 to
bb091bd
Compare
|
@rambleraptor I have updated with changes, please let me know if this needs improvement. Thanks! |
|
@kevinjqliu could you please review these changes, not sure ci check fails are related to this. |
pyiceberg/expressions/__init__.py
Outdated
| class Not(IcebergBaseModel, BooleanExpression): | ||
| """NOT operation expression - logical negation.""" | ||
|
|
||
| child: BooleanExpression | ||
|
|
||
| def __new__(cls, child: BooleanExpression) -> BooleanExpression: # type: ignore | ||
| @model_validator(mode="before") | ||
| def _before(cls, values: Any) -> Any: | ||
| if isinstance(values, BooleanExpression): | ||
| return {"child": values} | ||
| return values | ||
|
|
||
| @model_validator(mode="after") | ||
| def _normalize(cls, model: Any) -> Any: | ||
| child = model.child | ||
| if child is AlwaysTrue(): | ||
| return AlwaysFalse() | ||
| elif child is AlwaysFalse(): | ||
| return AlwaysTrue() | ||
| elif isinstance(child, Not): | ||
| return child.child | ||
| obj = super().__new__(cls) | ||
| obj.child = child | ||
| return obj | ||
| return model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still need the __new__ method, how about:
class Not(IcebergBaseModel, BooleanExpression):
"""NOT operation expression - logical negation."""
model_config = ConfigDict(arbitrary_types_allowed=True)
type: TypingLiteral["not"] = Field(default="not")
child: BooleanExpression = Field()
def __init__(self, child: BooleanExpression, **_) -> None:
super().__init__(child=child)
def __new__(cls, child: BooleanExpression, **_) -> BooleanExpression: # type: ignore
if child is AlwaysTrue():
return AlwaysFalse()
elif child is AlwaysFalse():
return AlwaysTrue()
elif isinstance(child, Not):
return child.child
obj = super().__new__(cls)
return objThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, i'll update and test it.
Co-authored-by: Fokko Driesprong <fokko@apache.org>
Fokko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go! Thanks @Aniketsy for working on this, and thanks @rambleraptor for the review 🚀
#2520
This PR makes the
Notboolean expression inpyicebergJSON serializable using PydanticPlease let me know if my approach or fix needs any improvements . I’m open to feedback and happy to make changes based on suggestions.
Thank you !