Make SetPredicate and Subclasses JSON Serializable with Pydantic

[Aniketsy]

#2524

This PR addresses issue by making the SetPredicate class and its subclasses (In, NotIn) JSON serializable using Pydantic.

• Added tests to verify JSON serialization of In and NotIn predicates.

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 !

[Aniketsy]

@Fokko Thanks, for the feedback and correction . Please let me know if I need to do further improvements.

[Fokko]

@Aniketsy I've left a few more, but I think that should be it. We're super close!

[Aniketsy]

@Fokko Thanks! I’ve committed the suggested changes.

[Fokko]

@Aniketsy Thanks for the quick turnaround, the changes I've just suggested fix all the mypy errors for me locally. You still need to check out the branch locally and run make lint to fix all the formatting issues.

[Aniketsy]

@Fokko Thanks! Will run make lint locally and address the formatting issues.

[Aniketsy]

@Fokko I noticed the integration-test 3.12 CI is failing. I’m not sure what’s causing it, could you provide some guidance?

[Fokko]

This should be reverted:

Suggested change

	return NotIn[L](self.term, self.literals)
	return In[L](self.term, self.literals)

[Fokko]

The class and init should be:

class SetPredicate(IcebergBaseModel, UnboundPredicate[L], ABC):
    model_config = ConfigDict(arbitrary_types_allowed=True)

    type: TypingLiteral["in", "not-in"] = Field(default="in")
    literals: Set[Literal[L]] = Field(alias="items")

    def __init__(self, term: Union[str, UnboundTerm[Any]], literals: Union[Iterable[L], Iterable[Literal[L]]]):
        super().__init__(term=_to_unbound_term(term), items=_to_literal_set(literals))

[Fokko]

@Aniketsy The suggestions above work on my end. Sorry for this issue, it is more complicated than I anticipated

[Aniketsy]

Thanks @Fokko , I’ve committed the changes based on your suggestions. Do I need to check ruff format from my end .

[Fokko]

I've just kicked off the CI. I think we also need to merge #2564 first

[Aniketsy]

Got it, thanks for triggering the CI. I’ll wait for #2564 to be merged.

[kevinjqliu]

#2564 is merged, do you mind rebasing this PR?

[Aniketsy]

sure, i will do that.

[Fokko]

@Aniketsy I checked out your branch locally, and it looks like there is a bit more to do to get the test passing: #2572 😨

[Aniketsy]

@Fokko Could you please guide me on what I should do to get the test passing?

[kevinjqliu]

ruff (legacy alias)......................................................Failed
- hook id: ruff
- exit code: 1
- files were modified by this hook

F821 Undefined name `IcebergBaseModel`
   --> pyiceberg/expressions/__init__.py:580:41
    |
580 | class SetPredicate(UnboundPredicate[L], IcebergBaseModel, ABC):
    |                                         ^^^^^^^^^^^^^^^^
581 |     model_config = ConfigDict(arbitrary_types_allowed=True)
    |

Found 3 errors (2 fixed, 1 remaining).

perhaps this is an import issue. can you try make lint / make test locally?

[Aniketsy]

Hi @kevinjqliu I’ve fixed the error. Could you please take a look?

[kevinjqliu]

#2575 is merged, lets see if this can block the PR

[Fokko]

@Aniketsy I checked out the branch locally, and with the following changes the tests should pass:

➜  iceberg-python git:(fix/setpredicate-json-serialization) ✗ git diff

diff --git a/pyiceberg/expressions/__init__.py b/pyiceberg/expressions/__init__.py
index 00cb404bb..d0824cc31 100644
--- a/pyiceberg/expressions/__init__.py
+++ b/pyiceberg/expressions/__init__.py
@@ -577,15 +577,14 @@ class NotNaN(UnaryPredicate):
         return BoundNotNaN[L]
 
 
-class SetPredicate(UnboundPredicate[L], IcebergBaseModel, ABC):
+class SetPredicate(IcebergBaseModel, UnboundPredicate[L], ABC):
     model_config = ConfigDict(arbitrary_types_allowed=True)
 
-    type: TypingLiteral["in", "not-in"] = Field(default="in", alias="type")
+    type: TypingLiteral["in", "not-in"] = Field(default="in")
     literals: Set[Literal[L]] = Field(alias="items")
 
     def __init__(self, term: Union[str, UnboundTerm[Any]], literals: Union[Iterable[L], Iterable[Literal[L]]]):
-        super().__init__(term)
-        self.literals = _to_literal_set(literals)
+        super().__init__(term=_to_unbound_term(term), items=_to_literal_set(literals))  # type: ignore
 
     def bind(self, schema: Schema, case_sensitive: bool = True) -> BoundSetPredicate[L]:
         bound_term = self.term.bind(schema, case_sensitive)
@@ -737,7 +736,7 @@ class NotIn(SetPredicate[L], ABC):
 
     def __invert__(self) -> In[L]:
         """Transform the Expression into its negated version."""
-        return NotIn[L](self.term, self.literals)
+        return In[L](self.term, self.literals)
 
     @property
     def as_bound(self) -> Type[BoundNotIn[L]]:
diff --git a/tests/expressions/test_expressions.py b/tests/expressions/test_expressions.py
index 5123a62ea..5a0c8c924 100644
--- a/tests/expressions/test_expressions.py
+++ b/tests/expressions/test_expressions.py
@@ -875,12 +875,12 @@ def test_not_in() -> None:
 
 def test_serialize_in() -> None:
     pred = In(term="foo", literals=[1, 2, 3])
-    assert pred.model_dump_json() == '{"type":"in","term":"foo","value":[1,2,3]}'
+    assert pred.model_dump_json() == '{"term":"foo","type":"in","items":[1,2,3]}'
 
 
 def test_serialize_not_in() -> None:
     pred = NotIn(term="foo", literals=[1, 2, 3])
-    assert pred.model_dump_json() == '{"type":"not-in","term":"foo","value":[1,2,3]}'
+    assert pred.model_dump_json() == '{"term":"foo","type":"not-in","items":[1,2,3]}'
 
 
 def test_bound_equal_to(term: BoundReference[Any]) -> None:

[Aniketsy]

@Fokko Thanks! I’ve applied the changes as per your suggestions.

[Aniketsy]

Thank you for guiding, reviewing, and correcting me at each step throughout this PR . @Fokko @kevinjqliu

[Fokko]

With pleasure, thank you for contributing 🙌