kevinjqliu commented on code in PR #2561:
URL: https://github.com/apache/iceberg-python/pull/2561#discussion_r2443399018
##########
pyiceberg/transforms.py:
##########
@@ -120,7 +120,7 @@ def _try_import(module_name: str, extras_name:
Optional[str] = None) -> types.Mo
raise NotInstalledError(msg) from None
-def _transform_literal(func: Callable[[L], L], lit: Literal[L]) -> Literal[L]:
+def _transform_literal(func: Callable[[Any], Any], lit: Literal[L]) ->
Literal[L]:
Review Comment:
nit: is this change relevant? i dont see `_transform_literal` used anywhere
##########
tests/expressions/test_expressions.py:
##########
@@ -1199,7 +1218,12 @@ def test_bind_ambiguous_name() -> None:
# |_| |_|\_, |_| \_, |
# |__/ |__/
-assert_type(EqualTo("a", "b"), EqualTo[str])
+
+def _assert_literal_predicate_type(expr: LiteralPredicate[L]) -> None:
+ assert_type(expr, LiteralPredicate[L])
+
+
+_assert_literal_predicate_type(EqualTo("a", "b"))
assert_type(In("a", ("a", "b", "c")), In[str])
assert_type(In("a", (1, 2, 3)), In[int])
assert_type(NotIn("a", ("a", "b", "c")), NotIn[str])
Review Comment:
nit: should we use `_assert_literal_predicate_type` for these too?
##########
pyiceberg/expressions/__init__.py:
##########
@@ -743,12 +743,18 @@ def as_bound(self) -> Type[BoundNotIn[L]]:
return BoundNotIn[L]
-class LiteralPredicate(UnboundPredicate[L], ABC):
- literal: Literal[L]
+class LiteralPredicate(IcebergBaseModel, UnboundPredicate[L], ABC):
+ type: TypingLiteral["lt", "lt-eq", "gt", "gt-eq", "eq", "not-eq",
"starts-with", "not-starts-with"] = Field(alias="type")
Review Comment:
👍
this matches the `LiteralExpression` definition
https://github.com/apache/iceberg/blob/b987e60bbd581d6e9e583107d5a85022261ff0d8/open-api/rest-catalog-open-api.yaml#L2264
##########
pyiceberg/expressions/__init__.py:
##########
@@ -743,12 +743,18 @@ def as_bound(self) -> Type[BoundNotIn[L]]:
return BoundNotIn[L]
-class LiteralPredicate(UnboundPredicate[L], ABC):
- literal: Literal[L]
+class LiteralPredicate(IcebergBaseModel, UnboundPredicate[L], ABC):
+ type: TypingLiteral["lt", "lt-eq", "gt", "gt-eq", "eq", "not-eq",
"starts-with", "not-starts-with"] = Field(alias="type")
Review Comment:
nit: could you include this in the PR description so its easily referenced
in the future?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
