You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Aniketsy (via GitHub)" <gi...@apache.org> on 2025/10/08 05:44:03 UTC
[PR] Make Not expression JSON serializable [iceberg-python]
Aniketsy opened a new pull request, #2593:
URL: https://github.com/apache/iceberg-python/pull/2593
#2520
This PR makes the `Not` boolean expression in `pyiceberg` JSON serializable using Pydantic
Please 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 !
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make Not expression JSON serializable [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2593:
URL: https://github.com/apache/iceberg-python/pull/2593#discussion_r2454682625
##########
pyiceberg/expressions/__init__.py:
##########
@@ -341,20 +341,25 @@ def __getnewargs__(self) -> Tuple[BooleanExpression, BooleanExpression]:
return (self.left, self.right)
-class Not(BooleanExpression):
+class Not(IcebergBaseModel, BooleanExpression):
"""NOT operation expression - logical negation."""
- child: BooleanExpression
+ model_config = ConfigDict(arbitrary_types_allowed=True)
+
+ type: TypingLiteral["not"] = Field(default="not")
+ child: BooleanExpression = Field()
+
+ def __init__(self, child: BooleanExpression, **_: Any) -> None:
+ super().__init__(child=child)
- def __new__(cls, child: BooleanExpression) -> BooleanExpression: # type: ignore
+ def __new__(cls, child: BooleanExpression, **_: Any) -> 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)
- obj.child = child
return obj
def __repr__(self) -> str:
Review Comment:
```suggestion
def __str__(self) -> str:
"""Return the string representation of the Not class."""
return f"Not(child={self.child})"
def __repr__(self) -> str:
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make Not expression JSON serializable [iceberg-python]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #2593:
URL: https://github.com/apache/iceberg-python/pull/2593#discussion_r2418002223
##########
pyiceberg/expressions/__init__.py:
##########
@@ -32,7 +32,7 @@
Union,
)
-from pydantic import Field
Review Comment:
You still need this import
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make Not expression JSON serializable [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2593:
URL: https://github.com/apache/iceberg-python/pull/2593#discussion_r2454460428
##########
pyiceberg/expressions/__init__.py:
##########
@@ -35,6 +35,8 @@
from pydantic import Field
+from pydantic import Field
+
Review Comment:
Duplicate import
```suggestion
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make Not expression JSON serializable [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2593:
URL: https://github.com/apache/iceberg-python/pull/2593#discussion_r2454474684
##########
pyiceberg/expressions/__init__.py:
##########
@@ -341,20 +341,25 @@ def __getnewargs__(self) -> Tuple[BooleanExpression, BooleanExpression]:
return (self.left, self.right)
-class Not(BooleanExpression):
+class Not(IcebergBaseModel, BooleanExpression):
"""NOT operation expression - logical negation."""
- child: BooleanExpression
+ model_config = ConfigDict(arbitrary_types_allowed=True)
+
+ type: TypingLiteral["not"] = Field(default="not")
+ child: BooleanExpression = Field()
+
+ def __init__(self, child: BooleanExpression, **_) -> None:
Review Comment:
```suggestion
def __init__(self, child: BooleanExpression, **_: Any) -> None:
```
##########
pyiceberg/expressions/__init__.py:
##########
@@ -341,20 +341,25 @@ def __getnewargs__(self) -> Tuple[BooleanExpression, BooleanExpression]:
return (self.left, self.right)
-class Not(BooleanExpression):
+class Not(IcebergBaseModel, BooleanExpression):
"""NOT operation expression - logical negation."""
- child: BooleanExpression
+ 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
+ def __new__(cls, child: BooleanExpression, **_) -> BooleanExpression: # type: ignore
Review Comment:
```suggestion
def __new__(cls, child: BooleanExpression, **_: Any) -> BooleanExpression: # type: ignore
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make Not expression JSON serializable [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2593:
URL: https://github.com/apache/iceberg-python/pull/2593#discussion_r2449286167
##########
pyiceberg/expressions/__init__.py:
##########
@@ -341,21 +343,27 @@ def __getnewargs__(self) -> Tuple[BooleanExpression, BooleanExpression]:
return (self.left, self.right)
-class Not(BooleanExpression):
+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
Review Comment:
I think we still need the `__new__` method, how about:
```python
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
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make Not expression JSON serializable [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2593:
URL: https://github.com/apache/iceberg-python/pull/2593#discussion_r2449284683
##########
tests/expressions/test_expressions.py:
##########
@@ -738,6 +738,29 @@ def test_not() -> None:
assert not_ == pickle.loads(pickle.dumps(not_))
+def test_not_json_serialization_and_deserialization() -> None:
+ expr = Not(AlwaysFalse())
+ json_str = expr.model_dump_json()
+ restored = Not.model_validate_json(json_str)
+ assert isinstance(restored, AlwaysTrue)
+
+ expr2 = Not(Not(AlwaysFalse()))
+ json_str2 = expr2.model_dump_json()
+ restored2 = Not.model_validate_json(json_str2)
+ assert isinstance(restored2, AlwaysFalse)
+
+ class DummyExpr(BooleanExpression):
+ def __invert__(self) -> BooleanExpression:
+ return self
+
+ dummy = DummyExpr()
+ not_dummy = Not(child=dummy)
+ json_str3 = not_dummy.model_dump_json()
+ restored3 = Not.model_validate_json(json_str3)
+ assert isinstance(restored3, Not)
+ assert isinstance(restored3.child, DummyExpr)
Review Comment:
Let's make this test much simpler for now. `Not(AlwaysFalse())` will automatically be rewritten to `AlwaysTrue()`, so the test is not a real round trip. Let simplify it to:
```suggestion
def test_not_json_serialization_and_deserialization() -> None:
not_expr = Not(GreaterThan("a", 22))
json_str = not_expr.model_dump_json()
assert json_str == """{"type":"not","child":{"term":"a","type":"gt","value":22}}"""
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make Not expression JSON serializable [iceberg-python]
Posted by "Aniketsy (via GitHub)" <gi...@apache.org>.
Aniketsy commented on code in PR #2593:
URL: https://github.com/apache/iceberg-python/pull/2593#discussion_r2449345723
##########
pyiceberg/expressions/__init__.py:
##########
@@ -341,21 +343,27 @@ def __getnewargs__(self) -> Tuple[BooleanExpression, BooleanExpression]:
return (self.left, self.right)
-class Not(BooleanExpression):
+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
Review Comment:
sure, i'll update and test it.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make Not expression JSON serializable [iceberg-python]
Posted by "Aniketsy (via GitHub)" <gi...@apache.org>.
Aniketsy commented on code in PR #2593:
URL: https://github.com/apache/iceberg-python/pull/2593#discussion_r2449345723
##########
pyiceberg/expressions/__init__.py:
##########
@@ -341,21 +343,27 @@ def __getnewargs__(self) -> Tuple[BooleanExpression, BooleanExpression]:
return (self.left, self.right)
-class Not(BooleanExpression):
+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
Review Comment:
sure, i'll update and test it
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make Not expression JSON serializable [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2593:
URL: https://github.com/apache/iceberg-python/pull/2593#discussion_r2454412995
##########
pyiceberg/expressions/__init__.py:
##########
@@ -35,6 +35,8 @@
from pydantic import Field
+from pydantic import Field, model_validator
Review Comment:
```suggestion
from pydantic import Field
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make Not expression JSON serializable [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2593:
URL: https://github.com/apache/iceberg-python/pull/2593#discussion_r2454460428
##########
pyiceberg/expressions/__init__.py:
##########
@@ -35,6 +35,8 @@
from pydantic import Field
+from pydantic import Field
+
Review Comment:
Duplicate import
```suggestion
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make Not expression JSON serializable [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #2593:
URL: https://github.com/apache/iceberg-python/pull/2593#discussion_r2454461821
##########
pyiceberg/expressions/__init__.py:
##########
@@ -35,6 +35,8 @@
from pydantic import Field
+from pydantic import Field
+
from pyiceberg.expressions.literals import (
Review Comment:
```suggestion
from pyiceberg.expressions.literals import (
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make Not expression JSON serializable [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #2593:
URL: https://github.com/apache/iceberg-python/pull/2593
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make Not expression JSON serializable [iceberg-python]
Posted by "Aniketsy (via GitHub)" <gi...@apache.org>.
Aniketsy commented on PR #2593:
URL: https://github.com/apache/iceberg-python/pull/2593#issuecomment-3422954429
@kevinjqliu could you please review these changes, not sure ci check fails are related to this.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Make Not expression JSON serializable [iceberg-python]
Posted by "Aniketsy (via GitHub)" <gi...@apache.org>.
Aniketsy commented on PR #2593:
URL: https://github.com/apache/iceberg-python/pull/2593#issuecomment-3388233263
@rambleraptor I have updated with changes, please let me know if this needs improvement. Thanks!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org