Fokko commented on code in PR #8286:
URL: https://github.com/apache/iceberg/pull/8286#discussion_r1290115198
##########
python/pyiceberg/catalog/rest.py:
##########
@@ -113,7 +113,7 @@ class Endpoints:
class TableResponse(IcebergBaseModel):
metadata_location: str = Field(alias="metadata-location")
- metadata: TableMetadata = Field()
Review Comment:
Did you remove the `= Field()` for a specific reason?
##########
python/tests/table/test_metadata.py:
##########
@@ -76,12 +76,12 @@ def example_table_metadata_v1() -> Dict[str, Any]:
def test_from_dict_v1(example_table_metadata_v1: Dict[str, Any]) -> None:
"""Test initialization of a TableMetadata instance from a dictionary"""
- TableMetadataUtil.parse_obj(example_table_metadata_v1)
+ TableMetadataUtil.parse_raw(json.dumps(example_table_metadata_v1))
Review Comment:
I think for testing we're okay with leaving as it was.
##########
python/pyiceberg/table/metadata.py:
##########
@@ -353,7 +356,7 @@ def check_sort_orders(cls, values: Dict[str, Any]) ->
Dict[str, Any]:
increasing long that tracks the order of snapshots in a table."""
-TableMetadata = Union[TableMetadataV1, TableMetadataV2]
+TableMetadata = Annotated[Union[TableMetadataV1, TableMetadataV2],
Field(discriminator="format_version")]
Review Comment:
Love it
##########
python/pyiceberg/table/metadata.py:
##########
@@ -380,19 +383,14 @@ def new_table_metadata(
class TableMetadataUtil:
"""Helper class for parsing TableMetadata."""
- # Once this has been resolved, we can simplify this:
https://github.com/samuelcolvin/pydantic/issues/3846
- # TableMetadata = Annotated[TableMetadata, Field(alias="format-version",
discriminator="format-version")]
+ class _MetadataWrapper(IcebergBaseModel):
+ table_metadata: TableMetadata
@staticmethod
- def parse_obj(data: Dict[str, Any]) -> TableMetadata:
- if "format-version" not in data:
- raise ValidationError(f"Missing format-version in TableMetadata:
{data}")
-
- format_version = data["format-version"]
-
- if format_version == 1:
- return TableMetadataV1(**data)
- elif format_version == 2:
- return TableMetadataV2(**data)
- else:
- raise ValidationError(f"Unknown format version: {format_version}")
+ def parse_raw(data: str) -> TableMetadata:
+ try:
+ d = f'{{"table_metadata": {data}}}'
+ metadata_wrapper = TableMetadataUtil._MetadataWrapper.parse_raw(d)
Review Comment:
The use of the wrapper doesn't make sense to me. Shouldn't
```suggestion
metadata_wrapper = TableMetadata.parse_raw(d)
```
work?
##########
python/pyiceberg/table/metadata.py:
##########
@@ -303,7 +306,7 @@ def to_v2(self) -> "TableMetadataV2":
metadata["format_version"] = 2
return TableMetadataV2(**metadata)
- format_version: Literal[1] = Field(alias="format-version")
+ format_version: Literal[1] = Field(alias="format-version", default=1)
Review Comment:
I don't think we can set a default here, it is missing, we should raise an
exception.
--
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]
