paulcichonski commented on code in PR #1432: URL: https://github.com/apache/iceberg-python/pull/1432#discussion_r1887299466
########## pyiceberg/types.py: ########## @@ -328,8 +328,8 @@ def __init__( data["type"] = data["type"] if "type" in data else field_type data["required"] = required data["doc"] = doc Review Comment: That could work, my only worry would be that ordinarily minor changes like changing field order in the class definition would break consumers in unexpected ways. For example, this instantiation works fine: ```python from pyiceberg.types import NestedField, StringType >>> NestedField(1, 'test', StringType(), True) NestedField(field_id=1, name='test', field_type=StringType(), required=True) ``` If someone (for some reason) changed field order or adds a new field in the middle, for example: ```diff diff --git a/pyiceberg/types.py b/pyiceberg/types.py index bd0eb7a..f17a3d4 100644 --- a/pyiceberg/types.py +++ b/pyiceberg/types.py @@ -304,33 +304,22 @@ class NestedField(IcebergType): field_id: int = Field(alias="id") name: str = Field() - field_type: SerializeAsAny[IcebergType] = Field(alias="type") required: bool = Field(default=False) + field_type: SerializeAsAny[IcebergType] = Field(alias="type") ``` Then the instantiation breaks: ```python >>> NestedField(1, 'test', StringType(), True) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/Users/pcichons/dev/code/cisco-sbgidm/iceberg-python/pyiceberg/types.py", line 322, in __init__ super().__init__(**kwargs) File "/Users/pcichons/dev/code/cisco-sbgidm/iceberg-python/.venv/lib/python3.12/site-packages/pydantic/main.py", line 214, in __init__ validated_self = self.__pydantic_validator__.validate_python(data, self_instance=self) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pydantic_core._pydantic_core.ValidationError: 2 validation errors for NestedField required Input should be a valid boolean [type=bool_type, input_value=StringType(), input_type=StringType] For further information visit https://errors.pydantic.dev/2.10/v/bool_type field_type Input should be a valid dictionary or instance of IcebergType [type=model_type, input_value=True, input_type=bool] For further information visit https://errors.pydantic.dev/2.10/v/model_type ``` Tests could catch that, but it might get a bit painful to deal with. -- 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