feat: make LiteralPredicate serializable via internal IcebergBaseModel

[jaimeferj]

Closes #2523

Rationale for this change

Are these changes tested?

yes

Are there any user-facing changes?

[jaimeferj]

In the issue #2523 it is said to derive the class from IcebergBaseModel which I have not done, but could try on if my solution is not accepted.

[jaimeferj]

I have now marked it as a Draft since I am not sure now that is the kind of implementation you want. Tests are still passing now using LiteralPredicate as subclass of IcebergBaseModel, but had to make term a Term[L] instead of what I thought it was (UnboundTerm[L]) because test_not_equal_to_invert and other ones of the same kind would fail otherwise, since they are using a BoundedTerm instead.

[jaimeferj]

Something fishy that I had to pull in order for tests to pass was putting the attribute term as Term[L] instead of UnboundTerm[Any] as it was in UnboundPredicate, father of LiteralPredicate. However, that also is triggering mypy since we are changing types from father to child.

The problem is that the earlier implementation was calling _to_unbound_term when initializing the instance, however, it does not always return UnboundTerm as you can easily see from the implementation:

def _to_unbound_term(term: Union[str, UnboundTerm[Any]]) -> UnboundTerm[Any]:
    return Reference(term) if isinstance(term, str) else term

If term is not UnboundTerm nor str the output is whatever the input was. For example, as done in test test_not_equal_to_invert because it is being initialized with a BoundRefence! If you use in the Pydantic model term as UnboundTerm[L]:

_______________________________________________ test_not_equal_to_invert _______________________________________________

    def test_not_equal_to_invert() -> None:
>       bound = NotEqualTo(
            term=BoundReference(  # type: ignore
                field=NestedField(field_id=1, name="foo", field_type=StringType(), required=False),
                accessor=Accessor(position=0, inner=None),
            ),
            literal="hello",
        )

Should we address this in this PR or delegate it to another issue?

[Fokko]

Should we address this in this PR or delegate it to another issue?

That's a bit of an edge case, since you deliberately ignore the type annotation. We could add a check in the function itself:

def _to_unbound_term(term: Union[str, UnboundTerm[Any]]) -> UnboundTerm[Any]:
    if isinstance(term, str),
        return Reference(term)
    elif isinstance(term, UnboundTerm):
        return term
    else:
        raise ValueError(f"Expected UnboundTerm or str, but got: {term}")
    return Reference(term) if isinstance(term, str) else term

[jaimeferj]

def _to_unbound_term(term: Union[str, UnboundTerm[Any]]) -> UnboundTerm[Any]:
    if isinstance(term, str),
        return Reference(term)
    elif isinstance(term, UnboundTerm):
        return term
    else:
        raise ValueError(f"Expected UnboundTerm or str, but got: {term}")
    return Reference(term) if isinstance(term, str) else term

I do not ignore the type annotation, it is the current implementation and a current test that is ignoring the annotation. I am trying to just implement the issue requirement and since now the types are checked in runtime by pydantic the (old) test is not passing.

[jaimeferj]

After having many issues with an init such as:

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))

Because there are some typing errors with _transform_literal in pyiceberg/transforms.py for example:

  pyiceberg/transforms.py:1113: error: Argument 1 to "_transform_literal" has incompatible type "Callable[[str | None], str | None]"; expected "Callable[[str], str]"  [arg-type]
  pyiceberg/transforms.py:1113: error: Argument 1 to "_transform_literal" has incompatible type "Callable[[bool | None], bool | None]"; expected "Callable[[str], str]"  [arg-type]
  pyiceberg/transforms.py:1113: error: Argument 1 to "_transform_literal" has incompatible type "Callable[[int | None], int | None]"; expected "Callable[[str], str]"  [arg-type]
  pyiceberg/transforms.py:1113: error: Argument 1 to "_transform_literal" has incompatible type "Callable[[float | None], float | None]"; expected "Callable[[str], str]"  [arg-type]
  pyiceberg/transforms.py:1113: error: Argument 1 to "_transform_literal" has incompatible type "Callable[[bytes | None], bytes | None]"; expected "Callable[[str], str]"  [arg-type]
  pyiceberg/transforms.py:1113: error: Argument 1 to "_transform_literal" has incompatible type "Callable[[UUID | None], UUID | None]"; expected "Callable[[str], str]"  [arg-type]

I decided to just go for this implementation of init. The problem now is that:

assert_type(EqualTo("a", "b"), EqualTo[str])  # <-- Fails
------
  tests/expressions/test_expressions.py:1238: error: Expression is of type "LiteralPredicate[L]", not "EqualTo[str]"  [assert-type]

So I am really stuck, would you mind lending a hand here? @Fokko

[Fokko]

Always! So, I think the linter isn't really sure what to do. It is pretty clear:

In the signature, we see that transform accepts an optional, which I think is correct. However, _transform_literal requires non-null, which is incorrect.

def _truncate_number(
    name: str, pred: BoundLiteralPredicate[L], transform: Callable[[Optional[L]], Optional[L]]
) -> Optional[UnboundPredicate[Any]]:
    boundary = pred.literal

    if not isinstance(boundary, (LongLiteral, DecimalLiteral, DateLiteral, TimestampLiteral)):
        raise ValueError(f"Expected a numeric literal, got: {type(boundary)}")

    if isinstance(pred, BoundLessThan):
        return LessThanOrEqual(Reference(name), _transform_literal(transform, boundary.decrement()))
    elif isinstance(pred, BoundLessThanOrEqual):

The following change suppresses most of the warnings for me:

-def _transform_literal(func: Callable[[L], L], lit: Literal[L]) -> Literal[L]:
+def _transform_literal(func: Callable[[Any], Any], lit: Literal[L]) -> Literal[L]:
     """Small helper to upwrap the value from the literal, and wrap it again."""
     return literal(func(lit.value))

[Fokko]

Suggested change

	literal: Literal[L] = Field(serialization_alias="value")
	value: Literal[L] = Field()

[Fokko]

If we just call the field value, and we add a literal property:

    @property
    def literal(self) -> Literal[L]:
        return self.value

[Fokko]

Always! So, I think the linter isn't really sure what to do. It is pretty clear:

In the signature, we see that transform accepts an optional, which I think is correct. However, _transform_literal requires non-null, which is incorrect.

def _truncate_number(
    name: str, pred: BoundLiteralPredicate[L], transform: Callable[[Optional[L]], Optional[L]]
) -> Optional[UnboundPredicate[Any]]:
    boundary = pred.literal

    if not isinstance(boundary, (LongLiteral, DecimalLiteral, DateLiteral, TimestampLiteral)):
        raise ValueError(f"Expected a numeric literal, got: {type(boundary)}")

    if isinstance(pred, BoundLessThan):
        return LessThanOrEqual(Reference(name), _transform_literal(transform, boundary.decrement()))
    elif isinstance(pred, BoundLessThanOrEqual):

The following change suppresses most of the warnings for me:

-def _transform_literal(func: Callable[[L], L], lit: Literal[L]) -> Literal[L]:
+def _transform_literal(func: Callable[[Any], Any], lit: Literal[L]) -> Literal[L]:
     """Small helper to upwrap the value from the literal, and wrap it again."""
     return literal(func(lit.value))