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

Reply via email to