You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "HonahX (via GitHub)" <gi...@apache.org> on 2023/12/05 05:22:33 UTC
[PR] Use Pydantic's `model_copy` for model modification when updating table metadata [iceberg-python]
HonahX opened a new pull request, #182:
URL: https://github.com/apache/iceberg-python/pull/182
Fixes #179
This PR uses Pydantic's [`model_copy`](https://docs.pydantic.dev/latest/api/base_model/#pydantic.main.BaseModel.model_copy) to apply table updates to metadata. Specifically:
- When applying `AddSchema`, `SetCurrentSchema`, `AddSnapshot`, `SetSnapshotRef`, we use `base_metadata.model_copy(update=...)` to get the updated metadata.
- When applying `UpgradeFormatVersion`, we still need to rebuild the whole model from `TableMetadataV1` to `TableMetadataV2`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Use Pydantic's `model_copy` for model modification when updating table metadata [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #182:
URL: https://github.com/apache/iceberg-python/pull/182#discussion_r1415525877
##########
pyiceberg/table/__init__.py:
##########
@@ -533,6 +535,8 @@ def update_table_metadata(base_metadata: TableMetadata, updates: Tuple[TableUpda
for update in updates:
new_metadata = _apply_table_update(update, new_metadata, context)
+ # Rebuild metadata to trigger validation
+ new_metadata = TableMetadataUtil.parse_obj(copy(new_metadata.model_dump()))
Review Comment:
I think we're okay to skip the validation on the Pydantic side, we should validate the input anyway to generate a more meaningful error.
We could set it to deep-copy using the `deep=True` kwarg. Doing this in Pydantic is probably an order of magnitude more efficient than doing this in Python.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Use Pydantic's `model_copy` for model modification when updating table metadata [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #182:
URL: https://github.com/apache/iceberg-python/pull/182#discussion_r1414899521
##########
pyiceberg/table/__init__.py:
##########
@@ -533,6 +535,8 @@ def update_table_metadata(base_metadata: TableMetadata, updates: Tuple[TableUpda
for update in updates:
new_metadata = _apply_table_update(update, new_metadata, context)
+ # Rebuild metadata to trigger validation
+ new_metadata = TableMetadataUtil.parse_obj(copy(new_metadata.model_dump()))
Review Comment:
Since `model_copy` performs a shallow copy by default, I believe we need to execute a deep copy before returning the final `new_metadata`. Otherwise, `base_metadata` might be inadvertently altered due to any improper updates applied to `new_metadata` subsequently.
Furthermore, as indicated by https://github.com/pydantic/pydantic/issues/418 and by some local tests, `model_copy(update=)` does not validate the contents of update. I think it might be good to reconstruct the metadata at this point to initiate the validation process.
(Alternatively, we could perform a deep copy here and incorporate the validation into our unit tests. Open to discussion on this approach.)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Use Pydantic's `model_copy` for model modification when updating table metadata [iceberg-python]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #182:
URL: https://github.com/apache/iceberg-python/pull/182#discussion_r1416622061
##########
pyiceberg/table/__init__.py:
##########
@@ -533,6 +535,8 @@ def update_table_metadata(base_metadata: TableMetadata, updates: Tuple[TableUpda
for update in updates:
new_metadata = _apply_table_update(update, new_metadata, context)
+ # Rebuild metadata to trigger validation
+ new_metadata = TableMetadataUtil.parse_obj(copy(new_metadata.model_dump()))
Review Comment:
Thanks for the explanation! I changed to `model_copy(deep=True)` here and moved the validation to a unit test
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Use Pydantic's `model_copy` for model modification when updating table metadata [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #182:
URL: https://github.com/apache/iceberg-python/pull/182
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org