laskoviymishka commented on code in PR #4321:
URL: https://github.com/apache/iceberg-python/pull/4321#discussion_r4519011453
##########
tests/catalog/test_rest.py:
##########
@@ -3713,3 +3716,52 @@ def test_load_table_without_storage_credentials(
)
assert actual.metadata.model_dump() == expected.metadata.model_dump()
assert actual == expected
+
+
+def test_load_responses_parse_labels(
+ example_table_metadata_with_snapshot_v1_rest_json: dict[str, Any],
example_view_metadata_rest_json: dict[str, Any]
+) -> None:
+ labels_json = {"object-labels": {"owner": "team-a"}, "fields":
[{"field-id": 1, "labels": {"classification": "pii"}}]}
+ expected = Labels(object_labels={"owner": "team-a"},
fields=[FieldLabel(field_id=1, labels={"classification": "pii"})])
+ cases: list[tuple[type[TableResponse | ViewResponse], dict[str, Any]]] = [
+ (TableResponse, example_table_metadata_with_snapshot_v1_rest_json),
+ (ViewResponse, example_view_metadata_rest_json),
+ ]
+ for response_type, payload in cases:
+ assert response_type.model_validate({**payload, "labels":
labels_json}).labels == expected
+ assert response_type.model_validate({**payload, "labels":
None}).labels == Labels()
+ assert response_type.model_validate(payload).labels == Labels()
+
+
+def test_load_table_with_labels(rest_mock: Mocker,
example_table_metadata_with_snapshot_v1_rest_json: dict[str, Any]) -> None:
+ rest_mock.get(
+ f"{TEST_URI}v1/namespaces/fokko/tables/table",
+ json={
+ **example_table_metadata_with_snapshot_v1_rest_json,
+ "labels": {"object-labels": {"owner": "team-a"}, "fields":
[{"field-id": 1, "labels": {"classification": "pii"}}]},
+ },
+ status_code=200,
+ request_headers=TEST_HEADERS,
+ )
+ catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN)
+ table = catalog.load_table(("fokko", "table"))
+
+ expected = Labels(object_labels={"owner": "team-a"},
fields=[FieldLabel(field_id=1, labels={"classification": "pii"})])
+ assert table.labels == expected
+
+ # Labels are not table state, so equality ignores them and refresh() keeps
the labels from the first load.
+ assert table == Table(
+ identifier=("fokko", "table"),
+
metadata_location=example_table_metadata_with_snapshot_v1_rest_json["metadata-location"],
+
metadata=TableMetadataV1(**example_table_metadata_with_snapshot_v1_rest_json["metadata"]),
+ io=load_file_io(),
+ catalog=catalog,
+ )
+ rest_mock.get(
+ f"{TEST_URI}v1/namespaces/fokko/tables/table",
+ json={**example_table_metadata_with_snapshot_v1_rest_json, "labels":
{"object-labels": {"owner": "team-b"}}},
+ status_code=200,
+ request_headers=TEST_HEADERS,
+ )
+ table.refresh()
+ assert table.labels == expected
Review Comment:
Could we pin the pickle behavior here? The description calls it the one
difference from Java, where the field is [`private final
transient`](https://github.com/apache/iceberg/blob/e8d0f0f3004a608e45986cbc2f8e5baf1a8a7db9/core/src/main/java/org/apache/iceberg/BaseTable.java#L51-L52),
and no test under `tests/` pickles a `Table` today.
```suggestion
assert table.labels == expected
assert pickle.loads(pickle.dumps(table)).labels == expected
```
This also needs `import pickle` at the top of the file.
##########
tests/catalog/test_rest.py:
##########
@@ -3713,3 +3716,52 @@ def test_load_table_without_storage_credentials(
)
assert actual.metadata.model_dump() == expected.metadata.model_dump()
assert actual == expected
+
+
+def test_load_responses_parse_labels(
+ example_table_metadata_with_snapshot_v1_rest_json: dict[str, Any],
example_view_metadata_rest_json: dict[str, Any]
+) -> None:
+ labels_json = {"object-labels": {"owner": "team-a"}, "fields":
[{"field-id": 1, "labels": {"classification": "pii"}}]}
+ expected = Labels(object_labels={"owner": "team-a"},
fields=[FieldLabel(field_id=1, labels={"classification": "pii"})])
+ cases: list[tuple[type[TableResponse | ViewResponse], dict[str, Any]]] = [
+ (TableResponse, example_table_metadata_with_snapshot_v1_rest_json),
+ (ViewResponse, example_view_metadata_rest_json),
+ ]
+ for response_type, payload in cases:
+ assert response_type.model_validate({**payload, "labels":
labels_json}).labels == expected
+ assert response_type.model_validate({**payload, "labels":
None}).labels == Labels()
+ assert response_type.model_validate(payload).labels == Labels()
+
+
+def test_load_table_with_labels(rest_mock: Mocker,
example_table_metadata_with_snapshot_v1_rest_json: dict[str, Any]) -> None:
+ rest_mock.get(
+ f"{TEST_URI}v1/namespaces/fokko/tables/table",
+ json={
+ **example_table_metadata_with_snapshot_v1_rest_json,
+ "labels": {"object-labels": {"owner": "team-a"}, "fields":
[{"field-id": 1, "labels": {"classification": "pii"}}]},
+ },
+ status_code=200,
+ request_headers=TEST_HEADERS,
+ )
+ catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN)
+ table = catalog.load_table(("fokko", "table"))
Review Comment:
Could this test also cover `create_table` and `register_table`? They hand
labels to the same `_response_to_table`
([create](https://github.com/apache/iceberg-python/blob/d211434ca1f69e9a7d36208d4e6401d068ac7b18/pyiceberg/catalog/rest/__init__.py#L1278),
[register](https://github.com/apache/iceberg-python/blob/d211434ca1f69e9a7d36208d4e6401d068ac7b18/pyiceberg/catalog/rest/__init__.py#L1377)),
but only load is exercised. Mocking the three responses and checking `labels`
in a `for` loop here would do it, per AGENTS.md's preference for a loop over
`parametrize`.
--
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]
