You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ragnard (via GitHub)" <gi...@apache.org> on 2025/12/26 22:18:23 UTC
[PR] fix: Validate SetStatisticsUpdate correctly (fixes #2865) [iceberg-python]
ragnard opened a new pull request, #2866:
URL: https://github.com/apache/iceberg-python/pull/2866
Previously the pydantic @model_validator would fail because it assumed statistics was a model instance. In a "before"" validator that is not the case.
Use an "after" validator instead, where we can use instantiated and validated fields.
### Before
```
>>> import pyiceberg.table.update
>>> pyiceberg.table.update.SetStatisticsUpdate.model_validate({'statistics': {'snapshot-id': 1234, 'file-size-in-bytes': 0, 'statistics-path': '', 'file-footer-size-in-bytes': 0, 'blob-metadata': []}})
Traceback (most recent call last):
File "<python-input-1>", line 1, in <module>
pyiceberg.table.update.SetStatisticsUpdate.model_validate({'statistics': {'snapshot-id': 1234, 'file-size-in-bytes': 0, 'statistics-path': '', 'file-footer-size-in-bytes': 0, 'blob-metadata': []}})
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/ragge/projects/github.com/ragnard/iceberg-python/.venv/lib/python3.14/site-packages/pydantic/main.py", line 716, in model_validate
return cls.__pydantic_validator__.validate_python(
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
obj,
^^^^
...<5 lines>...
by_name=by_name,
^^^^^^^^^^^^^^^^
)
^
File "/home/ragge/projects/github.com/ragnard/iceberg-python/pyiceberg/table/update/__init__.py", line 191, in validate_snapshot_id
data["snapshot_id"] = stats.snapshot_id
^^^^^^^^^^^^^^^^^
AttributeError: 'dict' object has no attribute 'snapshot_id'
```
### After
```
>>> import pyiceberg.table.update
>>> pyiceberg.table.update.SetStatisticsUpdate.model_validate({'statistics': {'snapshot-id': 1234, 'file-size-in-bytes': 0, 'statistics-path': '', 'file-footer-size-in-bytes': 0, 'blob-metadata': []}})
SetStatisticsUpdate(action='set-statistics', statistics=StatisticsFile(snapshot_id=1234, statistics_path='', file_size_in_bytes=0, file_footer_size_in_bytes=0, key_metadata=None, blob_metadata=[]), snapshot_id=1234)
```
# Rationale for this change
## Are these changes tested?
Yes, but only using the two-liners above.
## Are there any user-facing changes?
No.
<!-- In the case of user-facing changes, please add the changelog label. -->
--
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] fix: Validate SetStatisticsUpdate correctly (fixes #2865) [iceberg-python]
Posted by "ragnard (via GitHub)" <gi...@apache.org>.
ragnard commented on PR #2866:
URL: https://github.com/apache/iceberg-python/pull/2866#issuecomment-3697330087
> Looks like the java implementation has not deprecated the top-level `snapshot_id`. lets proceed with this change since it improves the current validation logic.
>
> Thanks for the PR! Lets add a test and it should be good to go!
@kevinjqliu Thanks for the (quick!) review. I've changed the fix a bit:
1) Since the model is frozen it is not possible to use direct assignment, but it is also not possible to use `model_copy` like I did because an "after" validator needs to return the same model instance. I've reverted to a "before" validator, but handle the `dict` case properly.
2) For testing, the issue is not really about tables with statistics, but that it was not possible to instantiate the `SetStatisticsUpdate` model from a dict. I added an `model_roundtrips` helper that is now used to check that any model roundtrips (`model -> dict -> model`) correctly.
Please let me know if you want further changes.
--
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] fix: Validate SetStatisticsUpdate correctly (fixes #2865) [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #2866:
URL: https://github.com/apache/iceberg-python/pull/2866#discussion_r2651736434
##########
pyiceberg/table/update/__init__.py:
##########
@@ -181,9 +181,15 @@ class SetStatisticsUpdate(IcebergBaseModel):
@model_validator(mode="before")
def validate_snapshot_id(cls, data: dict[str, Any]) -> dict[str, Any]:
- stats = cast(StatisticsFile, data["statistics"])
+ snapshot_id = None
- data["snapshot_id"] = stats.snapshot_id
+ stats = data["statistics"]
+ if isinstance(stats, StatisticsFile):
+ snapshot_id = stats.snapshot_id
+ elif isinstance(stats, dict):
+ snapshot_id = stats.get("snapshot_id")
+
Review Comment:
```suggestion
elif isinstance(stats, dict):
snapshot_id = stats.get("snapshot_id")
else:
snapshot_id = None
```
nit: i think we can inline the else here
##########
pyiceberg/table/update/__init__.py:
##########
@@ -181,9 +181,15 @@ class SetStatisticsUpdate(IcebergBaseModel):
@model_validator(mode="before")
def validate_snapshot_id(cls, data: dict[str, Any]) -> dict[str, Any]:
- stats = cast(StatisticsFile, data["statistics"])
+ snapshot_id = None
- data["snapshot_id"] = stats.snapshot_id
+ stats = data["statistics"]
+ if isinstance(stats, StatisticsFile):
+ snapshot_id = stats.snapshot_id
+ elif isinstance(stats, dict):
+ snapshot_id = stats.get("snapshot_id")
Review Comment:
```suggestion
snapshot_id = stats.get("snapshot-id")
```
i think this should be `snapshot-id` since before validator takes in json as input
https://github.com/apache/iceberg-python/blob/fa03e0814d8ed0a54be39e023f61e8163e37cdf3/pyiceberg/table/statistics.py#L32-L40
##########
pyiceberg/table/update/__init__.py:
##########
@@ -181,9 +181,15 @@ class SetStatisticsUpdate(IcebergBaseModel):
@model_validator(mode="before")
def validate_snapshot_id(cls, data: dict[str, Any]) -> dict[str, Any]:
- stats = cast(StatisticsFile, data["statistics"])
+ snapshot_id = None
- data["snapshot_id"] = stats.snapshot_id
+ stats = data["statistics"]
+ if isinstance(stats, StatisticsFile):
+ snapshot_id = stats.snapshot_id
+ elif isinstance(stats, dict):
+ snapshot_id = stats.get("snapshot_id")
Review Comment:
could you add a test case for this (and possibly one for the else case too)?
The current test only test the `StatisticsFile` instance branch
https://github.com/apache/iceberg-python/blob/fa03e0814d8ed0a54be39e023f61e8163e37cdf3/tests/table/test_init.py#L1370-L1381
--
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] fix: Validate SetStatisticsUpdate correctly (fixes #2865) [iceberg-python]
Posted by "ragnard (via GitHub)" <gi...@apache.org>.
ragnard commented on PR #2866:
URL: https://github.com/apache/iceberg-python/pull/2866#issuecomment-3693810506
@kevinjqliu Ok, do you want me to change the fix so that `snapshot_id` is still there, but just not automatically populated?
--
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] fix: Validate SetStatisticsUpdate correctly (fixes #2865) [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #2866:
URL: https://github.com/apache/iceberg-python/pull/2866#discussion_r2649878625
##########
pyiceberg/table/update/__init__.py:
##########
@@ -179,13 +179,9 @@ class SetStatisticsUpdate(IcebergBaseModel):
description="snapshot-id is **DEPRECATED for REMOVAL** since it contains redundant information. Use `statistics.snapshot-id` field instead.",
)
- @model_validator(mode="before")
- def validate_snapshot_id(cls, data: dict[str, Any]) -> dict[str, Any]:
- stats = cast(StatisticsFile, data["statistics"])
-
- data["snapshot_id"] = stats.snapshot_id
-
- return data
+ @model_validator(mode="after")
+ def validate_snapshot_id(self) -> Self:
+ return self.model_copy(update={"snapshot_id": self.statistics.snapshot_id})
Review Comment:
```suggestion
@model_validator(mode="after")
def validate_snapshot_id(self) -> Self:
self.snapshot_id = self.statistics.snapshot_id
return self
```
nit: use direct assignment
##########
pyiceberg/table/update/__init__.py:
##########
Review Comment:
add a test, something like:
```
def test_set_statistics_update_without_snapshot_id(table_v2_with_statistics: Table) -> None:
"""Test that SetStatisticsUpdate auto-populates snapshot_id from statistics."""
snapshot_id = table_v2_with_statistics.metadata.current_snapshot_id
blob_metadata = BlobMetadata(
type="apache-datasketches-theta-v1",
snapshot_id=snapshot_id,
sequence_number=2,
fields=[1],
properties={"prop-key": "prop-value"},
)
statistics_file = StatisticsFile(
snapshot_id=snapshot_id,
statistics_path="s3://bucket/warehouse/stats.puffin",
file_size_in_bytes=124,
file_footer_size_in_bytes=27,
blob_metadata=[blob_metadata],
)
# Create update without explicitly providing snapshot_id
update = SetStatisticsUpdate(statistics=statistics_file)
# Validator should auto-populate snapshot_id from statistics
assert update.snapshot_id == snapshot_id
new_metadata = update_table_metadata(
table_v2_with_statistics.metadata,
(update,),
)
assert len(new_metadata.statistics) == 2
updated_statistics = [stat for stat in new_metadata.statistics if stat.snapshot_id == snapshot_id]
assert len(updated_statistics) == 1
```
--
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] fix: Validate SetStatisticsUpdate correctly [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu merged PR #2866:
URL: https://github.com/apache/iceberg-python/pull/2866
--
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] fix: Validate SetStatisticsUpdate correctly (fixes #2865) [iceberg-python]
Posted by "ragnard (via GitHub)" <gi...@apache.org>.
ragnard commented on code in PR #2866:
URL: https://github.com/apache/iceberg-python/pull/2866#discussion_r2652589630
##########
pyiceberg/table/update/__init__.py:
##########
@@ -181,9 +181,15 @@ class SetStatisticsUpdate(IcebergBaseModel):
@model_validator(mode="before")
def validate_snapshot_id(cls, data: dict[str, Any]) -> dict[str, Any]:
- stats = cast(StatisticsFile, data["statistics"])
+ snapshot_id = None
- data["snapshot_id"] = stats.snapshot_id
+ stats = data["statistics"]
+ if isinstance(stats, StatisticsFile):
+ snapshot_id = stats.snapshot_id
+ elif isinstance(stats, dict):
+ snapshot_id = stats.get("snapshot_id")
Review Comment:
good catch with the `snapsht-id`. I've fixed that, and added a test specifically for the snapshot_id handling, creating a model both from a model instance, dict and json. the roundtrip testing also exercises the previous bug.
##########
pyiceberg/table/update/__init__.py:
##########
@@ -181,9 +181,15 @@ class SetStatisticsUpdate(IcebergBaseModel):
@model_validator(mode="before")
def validate_snapshot_id(cls, data: dict[str, Any]) -> dict[str, Any]:
- stats = cast(StatisticsFile, data["statistics"])
+ snapshot_id = None
- data["snapshot_id"] = stats.snapshot_id
+ stats = data["statistics"]
+ if isinstance(stats, StatisticsFile):
+ snapshot_id = stats.snapshot_id
+ elif isinstance(stats, dict):
+ snapshot_id = stats.get("snapshot_id")
Review Comment:
good catch with the `snapshot-id`. I've fixed that, and added a test specifically for the snapshot_id handling, creating a model both from a model instance, dict and json. the roundtrip testing also exercises the previous bug.
--
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] fix: Validate SetStatisticsUpdate correctly [iceberg-python]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #2866:
URL: https://github.com/apache/iceberg-python/pull/2866#issuecomment-3700303831
Thank you @ragnard for the PR and thanks @geruh for the review!
--
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