Skip to content

Commit 3dbf39f

Browse files
committed
Address review: validate format-version property strictly
Reject non-integer format-version values in set_properties with a clear error instead of truncating floats or failing with a confusing cast error, and verify in the integration test that the upgraded metadata persists and reloads from the catalog.
1 parent 9c5328c commit 3dbf39f

3 files changed

Lines changed: 14 additions & 0 deletions

File tree

‎pyiceberg/table/__init__.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -359,6 +359,8 @@ def set_properties(self, properties: Properties = EMPTY_DICT, **kwargs: Any) ->
359359
raise ValueError("Cannot pass both properties and kwargs")
360360
updates = dict(properties or kwargs)
361361
if (new_format_version := updates.pop(TableProperties.FORMAT_VERSION, None)) is not None:
362+
if isinstance(new_format_version, bool) or not str(new_format_version).isdigit():
363+
raise ValueError(f"Invalid format-version: {new_format_version}")
362364
self.upgrade_table_version(format_version=cast(TableVersion, int(new_format_version)))
363365
if updates:
364366
return self._apply((SetPropertiesUpdate(updates=updates),))

‎tests/integration/test_reads.py‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -977,6 +977,11 @@ def test_upgrade_table_version(catalog: Catalog) -> None:
977977
assert table_test_table_version.format_version == 3
978978
assert table_test_table_version.metadata.next_row_id == 0
979979

980+
# the upgraded metadata must persist and reload from the catalog
981+
reloaded = catalog.load_table("default.test_table_version")
982+
assert reloaded.format_version == 3
983+
assert reloaded.metadata.next_row_id == 0
984+
980985
with pytest.raises(ValueError) as e:
981986
with table_test_table_version.transaction() as transaction:
982987
transaction.upgrade_table_version(format_version=4) # type: ignore[arg-type]

‎tests/table/test_init.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2065,3 +2065,10 @@ def _spy(*args: Any, **kwargs: Any) -> FileIO:
20652065

20662066
assert seen_locations, "expected at least one load_file_io call"
20672067
assert all(loc is not None for loc in seen_locations), f"load_file_io called without a location: {seen_locations}"
2068+
2069+
2070+
def test_set_properties_invalid_format_version_rejected(table_v2: Table) -> None:
2071+
transaction = table_v2.transaction()
2072+
for invalid in ("3.0", "three", "-1"):
2073+
with pytest.raises(ValueError, match="Invalid format-version"):
2074+
transaction.set_properties({"format-version": invalid})

0 commit comments

Comments
 (0)