dragonknightk commented on code in PR #4209:
URL: https://github.com/apache/iceberg-python/pull/4209#discussion_r4434146848
##########
tests/table/test_init.py:
##########
@@ -1215,6 +1215,36 @@ def test_update_metadata_add_update_sort_order(table_v2:
Table) -> None:
assert new_metadata.last_updated_ms > table_v2.metadata.last_updated_ms
+def test_update_metadata_add_existing_sort_order(table_v2: Table) -> None:
+ existing_sort_order = table_v2.sort_order()
+ for order_id in (existing_sort_order.order_id,
existing_sort_order.order_id + 1):
+ new_metadata = update_table_metadata(
+ table_v2.metadata,
+ (
+
AddSortOrderUpdate(sort_order=SortOrder(*existing_sort_order.fields,
order_id=order_id)),
+ SetDefaultSortOrderUpdate(sort_order_id=-1),
+ ),
+ )
+ assert new_metadata.sort_orders == table_v2.metadata.sort_orders
+ assert new_metadata.default_sort_order_id ==
existing_sort_order.order_id
+
+
+def test_update_metadata_add_sort_order_with_taken_id(table_v2: Table) -> None:
+ existing_sort_order = table_v2.sort_order()
+ new_sort_order = SortOrder(
+ SortField(source_id=1, transform=IdentityTransform()),
+ order_id=existing_sort_order.order_id,
+ )
+ new_metadata = update_table_metadata(
+ table_v2.metadata,
+ (AddSortOrderUpdate(sort_order=new_sort_order),
SetDefaultSortOrderUpdate(sort_order_id=-1)),
+ )
+ assert len(new_metadata.sort_orders) == len(table_v2.metadata.sort_orders)
+ 1
+ assert new_metadata.sort_orders[-1].fields == new_sort_order.fields
+ assert new_metadata.sort_orders[-1].order_id ==
existing_sort_order.order_id + 1
+ assert new_metadata.default_sort_order_id == existing_sort_order.order_id
+ 1
Review Comment:
nit: the reused ids here are always the highest one, so recording `max(...)`
instead of the matched `sort.order_id` would still pass. The reuse path also
has to resolve to the matched order itself when its id is no longer the highest
— a writer that read metadata before a newer sort order appeared asks for the
older one with the id that newer order took.
```suggestion
assert new_metadata.default_sort_order_id ==
existing_sort_order.order_id + 1
def test_update_metadata_add_existing_sort_order_reuses_its_own_id(table_v2:
Table) -> None:
existing_sort_order = table_v2.sort_order()
newer_sort_order = SortOrder(SortField(source_id=1,
transform=IdentityTransform()), order_id=existing_sort_order.order_id + 1)
metadata = update_table_metadata(
table_v2.metadata,
(AddSortOrderUpdate(sort_order=newer_sort_order),
SetDefaultSortOrderUpdate(sort_order_id=-1)),
)
# A writer that read metadata before newer_sort_order was added picks
its own next id, which newer_sort_order took.
# Reusing existing_sort_order has to resolve to its own id, which is no
longer the highest one.
new_metadata = update_table_metadata(
metadata,
(
AddSortOrderUpdate(sort_order=SortOrder(*existing_sort_order.fields,
order_id=newer_sort_order.order_id + 1)),
SetDefaultSortOrderUpdate(sort_order_id=-1),
),
)
assert new_metadata.sort_orders == metadata.sort_orders
assert new_metadata.default_sort_order_id == existing_sort_order.order_id
```
##########
tests/table/test_init.py:
##########
@@ -1215,6 +1215,36 @@ def test_update_metadata_add_update_sort_order(table_v2:
Table) -> None:
assert new_metadata.last_updated_ms > table_v2.metadata.last_updated_ms
+def test_update_metadata_add_existing_sort_order(table_v2: Table) -> None:
+ existing_sort_order = table_v2.sort_order()
+ for order_id in (existing_sort_order.order_id,
existing_sort_order.order_id + 1):
+ new_metadata = update_table_metadata(
+ table_v2.metadata,
+ (
+
AddSortOrderUpdate(sort_order=SortOrder(*existing_sort_order.fields,
order_id=order_id)),
+ SetDefaultSortOrderUpdate(sort_order_id=-1),
+ ),
+ )
+ assert new_metadata.sort_orders == table_v2.metadata.sort_orders
+ assert new_metadata.default_sort_order_id ==
existing_sort_order.order_id
Review Comment:
nit: the reused ids here are all the current default, so the suite stays
green if the reuse path starts writing a metadata version. Asserting the
timestamp keeps it honest.
```suggestion
assert new_metadata.default_sort_order_id ==
existing_sort_order.order_id
assert new_metadata.last_updated_ms ==
table_v2.metadata.last_updated_ms
```
--
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]
