Skip to content

Commit a6df4e2

Browse files
committed
Point to the original name when moving a column renamed in the same update
Moves resolve names against the base schema, as in Java, so moving a column by its pending new name fails. Name the original column in the error and document it.
1 parent b32392d commit a6df4e2

3 files changed

Lines changed: 35 additions & 5 deletions

File tree

‎mkdocs/docs/api.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1242,6 +1242,8 @@ with table.update_schema() as update:
12421242
update.rename_column(("details", "confirmed_by"), "processed_by")
12431243
```
12441244

1245+
Within the same `update_schema()`, other operations still refer to a renamed column by its original name, for example `update.move_first("retries")` after the rename above.
1246+
12451247
### Move column
12461248

12471249
Move order of fields:

‎pyiceberg/table/update/schema.py‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -510,6 +510,19 @@ def _find_for_move(self, name: str) -> int | None:
510510

511511
return self._added_name_to_id.get(name)
512512

513+
def _missing_for_move(self, name: str) -> str:
514+
"""Describe a name that cannot be moved, pointing to the original name if it is a pending rename."""
515+
for field_id, update in self._updates.items():
516+
field = self._schema.find_field(field_id)
517+
if update.name == field.name:
518+
continue
519+
parent = self._id_to_parent.get(field_id)
520+
new_name = f"{parent}.{update.name}" if parent else update.name
521+
if new_name == name or (not self._case_sensitive and new_name.lower() == name.lower()):
522+
original_name = self._schema.find_column_name(field_id)
523+
return f"{name} (renamed from {original_name} in this update, refer to it as {original_name})"
524+
return name
525+
513526
def _move(self, move: _Move) -> None:
514527
if parent_name := self._id_to_parent.get(move.field_id):
515528
parent_field = self._schema.find_field(parent_name, case_sensitive=self._case_sensitive)
@@ -549,7 +562,7 @@ def move_first(self, path: str | tuple[str, ...]) -> UpdateSchema:
549562
field_id = self._find_for_move(full_name)
550563

551564
if field_id is None:
552-
raise ValueError(f"Cannot move missing column: {full_name}")
565+
raise ValueError(f"Cannot move missing column: {self._missing_for_move(full_name)}")
553566

554567
self._move(_Move(field_id=field_id, full_name=full_name, op=_MoveOperation.First))
555568

@@ -568,7 +581,7 @@ def move_before(self, path: str | tuple[str, ...], before_path: str | tuple[str,
568581
field_id = self._find_for_move(full_name)
569582

570583
if field_id is None:
571-
raise ValueError(f"Cannot move missing column: {full_name}")
584+
raise ValueError(f"Cannot move missing column: {self._missing_for_move(full_name)}")
572585

573586
before_full_name = (
574587
".".join(
@@ -580,7 +593,7 @@ def move_before(self, path: str | tuple[str, ...], before_path: str | tuple[str,
580593
before_field_id = self._find_for_move(before_full_name)
581594

582595
if before_field_id is None:
583-
raise ValueError(f"Cannot move {full_name} before missing column: {before_full_name}")
596+
raise ValueError(f"Cannot move {full_name} before missing column: {self._missing_for_move(before_full_name)}")
584597

585598
if field_id == before_field_id:
586599
raise ValueError(f"Cannot move {full_name} before itself")
@@ -603,13 +616,13 @@ def move_after(self, path: str | tuple[str, ...], after_name: str | tuple[str, .
603616
field_id = self._find_for_move(full_name)
604617

605618
if field_id is None:
606-
raise ValueError(f"Cannot move missing column: {full_name}")
619+
raise ValueError(f"Cannot move missing column: {self._missing_for_move(full_name)}")
607620

608621
after_path = ".".join(after_name) if isinstance(after_name, tuple) else after_name
609622
after_field_id = self._find_for_move(after_path)
610623

611624
if after_field_id is None:
612-
raise ValueError(f"Cannot move {full_name} after missing column: {after_path}")
625+
raise ValueError(f"Cannot move {full_name} after missing column: {self._missing_for_move(after_path)}")
613626

614627
if field_id == after_field_id:
615628
raise ValueError(f"Cannot move {full_name} after itself")

‎tests/test_schema.py‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1844,3 +1844,18 @@ def test_check_schema_compatible_optional_map_field_present() -> None:
18441844
)
18451845
# Should not raise - schemas match
18461846
_check_schema_compatible(requested_schema, provided_schema)
1847+
1848+
1849+
def test_move_renamed_column_by_new_name_points_to_original_name(table_v2: Table) -> None:
1850+
update = UpdateSchema(transaction=Transaction(table_v2)).rename_column("y", "y2")
1851+
1852+
with pytest.raises(ValueError, match=r"Cannot move missing column: y2 \(renamed from y in this update, refer to it as y\)"):
1853+
update.move_first("y2")
1854+
with pytest.raises(ValueError, match=r"before missing column: y2 \(renamed from y in this update, refer to it as y\)"):
1855+
update.move_before("x", "y2")
1856+
with pytest.raises(ValueError, match=r"after missing column: y2 \(renamed from y in this update, refer to it as y\)"):
1857+
update.move_after("x", "y2")
1858+
with pytest.raises(ValueError, match=r"^Cannot move missing column: unknown$"):
1859+
update.move_first("unknown")
1860+
1861+
assert [field.name for field in update.move_first("y")._apply().fields] == ["y2", "x", "z"]

0 commit comments

Comments
 (0)