-
Notifications
You must be signed in to change notification settings - Fork 378
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
base: main
Are you sure you want to change the base?
Conversation
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. |
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 obj
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.
sure, i'll update and test it.
Co-authored-by: Fokko Driesprong <[email protected]>
#2520
This PR makes the
Not
boolean expression inpyiceberg
JSON 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 !