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: [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]
