syun64 commented on code in PR #441:
URL: https://github.com/apache/iceberg-python/pull/441#discussion_r1495092898
##########
pyiceberg/table/name_mapping.py:
##########
@@ -45,6 +45,18 @@ class MappedField(IcebergBaseModel):
def convert_null_to_empty_List(cls, v: Any) -> Any:
return v or []
+ @field_validator('names', mode='before')
Review Comment:
Thank you for the suggestion. I've made the change 😄
##########
tests/table/test_name_mapping.py:
##########
@@ -238,3 +245,67 @@ def test_mapping_lookup_by_name(table_name_mapping_nested:
NameMapping) -> None:
with pytest.raises(ValueError, match="Could not find field with name:
boom"):
table_name_mapping_nested.find("boom")
+
+
+def test_invalid_mapped_field() -> None:
+ with pytest.raises(ValueError):
+ MappedField(field_id=1, names=[])
+
+
+def test_update_mapping_no_updates_or_adds(table_name_mapping_nested:
NameMapping) -> None:
+ assert update_mapping(table_name_mapping_nested, {}, {}) ==
table_name_mapping_nested
+
+
+def test_update_mapping(table_name_mapping_nested: NameMapping) -> None:
+ updates = {1: NestedField(1, "foo_update", StringType(), True)}
+ adds = {
+ -1: [NestedField(18, "add_18", StringType(), True)],
+ 15: [NestedField(19, "name", StringType(), True), NestedField(20,
"add_20", StringType(), True)],
+ }
Review Comment:
> Or when you reassign an existing name?
I think the "name" case covers the reassignment of the existing name.
> Should we also have checks in place when you add an existing ID?
This is an interesting one: Do you think we should add this case?
[UpdateSchema](https://github.com/apache/iceberg-python/blob/main/pyiceberg/table/__init__.py#L1608)
won't add existing IDs as it uses the `next(self._last_column_id)` to assign
the next field_id when a new column is added. The NameMapping class on its own
does not have a check to ensure that the field IDs across the MappedFields is
unique, and I wonder if we should bother introducing that contraint at the
class level.
--
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
