kevinjqliu commented on code in PR #1860:
URL: https://github.com/apache/iceberg-python/pull/1860#discussion_r2019120254
##########
tests/test_types.py:
##########
@@ -231,6 +231,22 @@ def test_nested_field() -> None:
assert "validation errors for NestedField" in str(exc_info.value)
+def test_nested_field_type_as_str_unsupported() -> None:
+ with pytest.raises(ValueError) as exc_info:
+ _ = (NestedField(1, "field", "list", required=True),)
Review Comment:
nit: lets also test other complex type `list/map/struct`
##########
pyiceberg/types.py:
##########
@@ -324,7 +324,7 @@ def __init__(
self,
field_id: Optional[int] = None,
name: Optional[str] = None,
- field_type: Optional[IcebergType] = None,
+ field_type: Optional[IcebergType | str] = None,
Review Comment:
nit: add to the example to the docstring above with `str` type
##########
tests/test_types.py:
##########
@@ -231,6 +231,22 @@ def test_nested_field() -> None:
assert "validation errors for NestedField" in str(exc_info.value)
+def test_nested_field_type_as_str_unsupported() -> None:
+ with pytest.raises(ValueError) as exc_info:
+ _ = (NestedField(1, "field", "list", required=True),)
+ assert "Unsupported field type: list" in str(exc_info.value)
+
+
+def test_nested_field_type_as_str_struct() -> None:
Review Comment:
nit: include other types that are part of `handle_primitive_type` here as
well
##########
pyiceberg/types.py:
##########
@@ -340,6 +340,12 @@ def __init__(
data["doc"] = doc
data["initial-default"] = data["initial-default"] if "initial-default"
in data else initial_default
data["write-default"] = data["write-default"] if "write-default" in
data else write_default
+ if isinstance(data["type"], str):
+ try:
+ data["type"] = IcebergType.handle_primitive_type(data["type"],
None)
+ except ValueError as e:
+ raise ValueError(f"Unsupported field type: {data['type']}.")
from e
+
Review Comment:
nit: what do you think about using a pydantic field validator here?
https://docs.pydantic.dev/latest/concepts/validators/#field-before-validator
```
@validator("field_type", pre=True)
def convert_field_type(cls, v):
"""Convert string values into IcebergType instances."""
if isinstance(v, str):
try:
return IcebergType.handle_primitive_type(v, None)
except ValueError as e:
raise ValueError(f"Unsupported field type: {v}") from e
return v # If already an IcebergType, keep as is
```
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
