kevinjqliu commented on code in PR #1426: URL: https://github.com/apache/iceberg-python/pull/1426#discussion_r1887165186
########## pyiceberg/table/name_mapping.py: ########## Review Comment: In `__str__` https://github.com/apache/iceberg-python/pull/1426/files#diff-55b6571bab1c711ef0bc63727c189fa6cb3ef5b38f0e6c4ade09f059fbc9960dR69 `(str(self.field_id) or "?")`, `str(None)` is `"None"`, we probably want `?` instead ########## pyiceberg/table/name_mapping.py: ########## @@ -333,8 +334,8 @@ def struct(self, struct: StructType, struct_partner: Optional[MappedField], fiel return StructType(*field_results) def field(self, field: NestedField, field_partner: Optional[MappedField], field_result: IcebergType) -> IcebergType: - if field_partner is None: - raise ValueError(f"Field missing from NameMapping: {'.'.join(self.current_path)}") + if field_partner is None or field_partner.field_id is None: Review Comment: I'm curious about this change, why do we need to check for `field_partner.field_id` ########## tests/table/test_name_mapping.py: ########## @@ -109,6 +109,21 @@ def test_json_mapped_field_no_names_deserialization() -> None: assert MappedField(field_id=1, names=[]) == MappedField.model_validate_json(mapped_field_with_null_fields) +def test_json_mapped_field_no_field_id_deserialization() -> None: + mapped_field = """{ + "names": [] + } + """ + assert MappedField(field_id=None, names=[]) == MappedField.model_validate_json(mapped_field) Review Comment: nit: also test omitting the `field_id=None` -- 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
