Skip to content

Commit b31b762

Browse files
fix(table): avoid committing update builders after exceptions (#3354)
## Summary Avoid committing update builders when the body of a `with` block raises. `UpdateTableMetadata.__exit__` currently commits unconditionally, so user code like `with table.update_schema() as update:` can still mutate table metadata even when an exception is raised before the block finishes. This change makes update builders mirror `Transaction.__exit__` and only commit on a clean exit. ## Testing - `.venv\\Scripts\\python -m pytest "tests/catalog/test_catalog_behaviors.py::test_update_schema_with_statement_does_not_commit_on_exception[memory]" "tests/catalog/test_catalog_behaviors.py::test_update_schema_with_statement_does_not_commit_on_exception[sql]" -q`
1 parent 739373f commit b31b762

2 files changed

Lines changed: 25 additions & 3 deletions

File tree

pyiceberg/table/update/__init__.py

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
from abc import ABC, abstractmethod
2222
from datetime import datetime
2323
from functools import singledispatch
24+
from types import TracebackType
2425
from typing import TYPE_CHECKING, Annotated, Any, Generic, Literal, TypeVar, cast
2526

2627
from pydantic import Field, field_validator, model_serializer, model_validator
@@ -71,9 +72,10 @@ def _commit(self) -> UpdatesAndRequirements: ...
7172
def commit(self) -> None:
7273
self._transaction._apply(*self._commit())
7374

74-
def __exit__(self, _: Any, value: Any, traceback: Any) -> None:
75-
"""Close and commit the change."""
76-
self.commit()
75+
def __exit__(self, exctype: type[BaseException] | None, excinst: BaseException | None, exctb: TracebackType | None) -> None:
76+
"""Close and commit the change if no exceptions have been raised."""
77+
if exctype is None and excinst is None and exctb is None:
78+
self.commit()
7779

7880
def __enter__(self) -> U:
7981
"""Update the table."""

tests/catalog/test_catalog_behaviors.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -942,6 +942,26 @@ def test_add_column_with_statement(catalog: Catalog, table_schema_simple: Schema
942942
assert table.schema().schema_id == 2
943943

944944

945+
def test_update_schema_with_statement_does_not_commit_on_exception(
946+
catalog: Catalog, table_schema_simple: Schema, random_table_identifier: Identifier
947+
) -> None:
948+
namespace = Catalog.namespace_from(random_table_identifier)
949+
catalog.create_namespace(namespace)
950+
table = catalog.create_table(random_table_identifier, table_schema_simple)
951+
952+
with pytest.raises(ValueError):
953+
with table.update_schema() as tx:
954+
tx.add_column(path="should_not_commit", field_type=IntegerType())
955+
int("boom")
956+
957+
assert table.schema() == table_schema_simple
958+
assert table.schema().schema_id == 0
959+
960+
reloaded = catalog.load_table(random_table_identifier)
961+
assert reloaded.schema() == table_schema_simple
962+
assert reloaded.schema().schema_id == 0
963+
964+
945965
# Namespace tests
946966

947967

0 commit comments

Comments
 (0)