kevinjqliu commented on code in PR #956:
URL: https://github.com/apache/iceberg-python/pull/956#discussion_r1687344048
##########
pyiceberg/table/__init__.py:
##########
@@ -1178,10 +1178,15 @@ def update_table_metadata(
"""
context = _TableMetadataUpdateContext()
new_metadata = base_metadata
+ new_metadata = new_metadata.model_copy(update={"last_updated_ms": 0})
for update in updates:
new_metadata = _apply_table_update(update, new_metadata, context)
Review Comment:
I think it might be more intuitive for the `_apply_table_update` to update
the `last_updated_ms` field as it updates the table metadata.
For example, in this
https://github.com/apache/iceberg-python/blob/3966263d935b92bcf98877ce9ba8ea26c08a283f/pyiceberg/table/__init__.py#L928-L934
there are checks to just return the table metadata without any updates
##########
tests/table/test_init.py:
##########
@@ -598,7 +598,7 @@ def test_apply_set_properties_update(table_v2: Table) ->
None:
base_metadata = table_v2.metadata
new_metadata_no_update = update_table_metadata(base_metadata,
(SetPropertiesUpdate(updates={}),))
- assert new_metadata_no_update == base_metadata
Review Comment:
since theres no updates to the metadata, I would expect the
`last_updated_ms` to not change
##########
tests/catalog/test_glue.py:
##########
@@ -707,6 +707,7 @@ def test_commit_table_update_schema(
test_catalog.create_namespace(namespace=database_name)
table = test_catalog.create_table(identifier, table_schema_nested)
original_table_metadata = table.metadata
+ last_update_ms = original_table_metadata.last_updated_ms
Review Comment:
nit: not necessary to check `last_update_ms` for all updates. perhaps just
when unit testing `update_table_metadata`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
