Skip to content

Commit b7ca7be

Browse files
authored
Reject empty source-ids in PartitionField / SortField (#3411)
Inspired by the walrus issue in #3353 ## Problem `PartitionField` and `SortField` accept the v3 `source-ids` list (added in [#1554](#1554)) and map it to the legacy singular `source-id`. Both validators try to reject an empty list: ```python if "source-id" not in data and (source_ids := data["source-ids"]): if isinstance(source_ids, list): if len(source_ids) == 0: raise ValueError("Empty source-ids is not allowed") ... data["source-id"] = source_ids[0] ``` The walrus uses truthiness, and `[]` is falsy — so the `len(source_ids) == 0` branch is unreachable. Passing `{"source-ids": []}` silently skips the mapping, and Pydantic then reports a generic "field required" error instead of the intended message. A missing `source-ids` key also raises `KeyError` instead of being handled cleanly. ## Fix Replace the walrus with an explicit key check in both validators: ```python if "source-id" not in data and "source-ids" in data: source_ids = data["source-ids"] ... ``` This makes the empty-list validation reachable and avoids the `KeyError`. ## Tests Added regression tests that deserialize `{"source-ids": []}` and assert `ValueError("Empty source-ids is not allowed")` is raised, for both `PartitionField` and `SortField`.
1 parent 720708a commit b7ca7be

4 files changed

Lines changed: 16 additions & 2 deletions

File tree

‎pyiceberg/partitioning.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,8 @@ def __init__(
109109
@classmethod
110110
def map_source_ids_onto_source_id(cls, data: Any) -> Any:
111111
if isinstance(data, dict):
112-
if "source-id" not in data and (source_ids := data["source-ids"]):
112+
if "source-id" not in data and "source-ids" in data:
113+
source_ids = data["source-ids"]
113114
if isinstance(source_ids, list):
114115
if len(source_ids) == 0:
115116
raise ValueError("Empty source-ids is not allowed")

‎pyiceberg/table/sorting.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,8 @@ def set_null_order(cls, values: dict[str, Any]) -> dict[str, Any]:
101101
@classmethod
102102
def map_source_ids_onto_source_id(cls, data: Any) -> Any:
103103
if isinstance(data, dict):
104-
if "source-id" not in data and (source_ids := data["source-ids"]):
104+
if "source-id" not in data and "source-ids" in data:
105+
source_ids = data["source-ids"]
105106
if isinstance(source_ids, list):
106107
if len(source_ids) == 0:
107108
raise ValueError("Empty source-ids is not allowed")

‎tests/table/test_partitioning.py‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,12 @@ def test_deserialize_partition_field_v3() -> None:
267267
assert field == PartitionField(source_id=1, field_id=1000, transform=TruncateTransform(width=19), name="str_truncate")
268268

269269

270+
def test_deserialize_partition_field_empty_source_ids_rejected() -> None:
271+
json_partition_spec = """{"source-ids": [], "field-id": 1000, "transform": "identity", "name": "x"}"""
272+
with pytest.raises(Exception, match="Empty source-ids is not allowed"):
273+
PartitionField.model_validate_json(json_partition_spec)
274+
275+
270276
def test_incompatible_source_column_not_found() -> None:
271277
schema = Schema(NestedField(1, "foo", IntegerType()), NestedField(2, "bar", IntegerType()))
272278

‎tests/table/test_sorting.py‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,12 @@ def test_serialize_sort_field_v3() -> None:
138138
assert SortField.model_validate_json(payload) == expected
139139

140140

141+
def test_deserialize_sort_field_empty_source_ids_rejected() -> None:
142+
payload = '{"source-ids":[],"transform":"identity","direction":"asc","null-order":"nulls-first"}'
143+
with pytest.raises(Exception, match="Empty source-ids is not allowed"):
144+
SortField.model_validate_json(payload)
145+
146+
141147
def test_incompatible_source_column_not_found(sort_order: SortOrder) -> None:
142148
schema = Schema(NestedField(1, "foo", IntegerType()), NestedField(2, "bar", IntegerType()))
143149

0 commit comments

Comments
 (0)