Skip to content

Commit 5133f35

Browse files
kevinjqliuCopilot
andauthored
Fix DataFile spec ID and reject unknown record fields (#3954)
* Fix DataFile spec ID binding Reject unknown record fields while retaining spec_id as transient DataFile context. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Fix DataFile integration fixture version argument Use the internal table format selector now that unknown DataFile fields are rejected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Use canonical DataFile in integration fixture Let the v2 manifest writer project the canonical record instead of constructing a version-specific positional layout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Exclude spec ID from manifest serialization fixture Keep transient DataFile context out of the expected Avro payload. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Clarify versioned DataFile test setup Keep the canonical DataFile name distinct from the explicit v1 record and order version selection first. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 308768d commit 5133f35

5 files changed

Lines changed: 38 additions & 10 deletions

File tree

‎pyiceberg/manifest.py‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -471,9 +471,14 @@ def data_file_with_partition(partition_type: StructType, format_version: TableVe
471471

472472
class DataFile(Record):
473473
@classmethod
474-
def from_args(cls, _table_format_version: TableVersion = DEFAULT_READ_VERSION, **arguments: Any) -> DataFile:
474+
def from_args(
475+
cls, _table_format_version: TableVersion = DEFAULT_READ_VERSION, *, spec_id: int | None = None, **arguments: Any
476+
) -> DataFile:
475477
struct = DATA_FILE_TYPE[_table_format_version]
476-
return super()._bind(struct, **arguments)
478+
data_file = super()._bind(struct, **arguments)
479+
if spec_id is not None:
480+
data_file.spec_id = spec_id
481+
return data_file
477482

478483
@property
479484
def content(self) -> DataFileContent:

‎pyiceberg/typedef.py‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,9 @@ class Record(StructProtocol):
179179

180180
@classmethod
181181
def _bind(cls, struct: StructType, **arguments: Any) -> Self:
182+
field_names = {field.name for field in struct.fields}
183+
if unknown_fields := arguments.keys() - field_names:
184+
raise TypeError(f"Unexpected {cls.__name__} fields: {', '.join(sorted(unknown_fields))}")
182185
return cls(*[arguments[field.name] if field.name in arguments else field.initial_default for field in struct.fields])
183186

184187
def __init__(self, *data: Any) -> None:

‎tests/avro/test_file.py‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -266,8 +266,7 @@ def test_write_v2_referenced_data_file_with_fastavro() -> None:
266266

267267
@pytest.mark.parametrize("format_version", [1, 2])
268268
def test_write_manifest_entry_with_fastavro_read_with_iceberg(format_version: TableVersion) -> None:
269-
data_file_dict = {
270-
"content": DataFileContent.DATA,
269+
common_data_file_args = {
271270
"file_path": "s3://some-path/some-file.parquet",
272271
"file_format": FileFormat.PARQUET,
273272
"partition": Record(),
@@ -281,16 +280,16 @@ def test_write_manifest_entry_with_fastavro_read_with_iceberg(format_version: Ta
281280
"upper_bounds": {1: b"zzzzzzzzzzzzzzzz"},
282281
"key_metadata": b"\xde\xad\xbe\xef",
283282
"split_offsets": [4, 133697593],
284-
"equality_ids": [],
285283
"sort_order_id": 4,
286284
"spec_id": 3,
287285
}
288-
data_file_v2 = DataFile.from_args(**data_file_dict) # type: ignore
286+
data_file = DataFile.from_args(content=DataFileContent.DATA, **common_data_file_args) # type: ignore
287+
assert data_file.spec_id == 3
289288

290289
entry = ManifestEntry.from_args(
291290
status=ManifestEntryStatus.ADDED,
292291
snapshot_id=8638475580105682862,
293-
data_file=data_file_v2,
292+
data_file=data_file,
294293
)
295294

296295
with TemporaryDirectory() as tmpdir:
@@ -322,7 +321,11 @@ def test_write_manifest_entry_with_fastavro_read_with_iceberg(format_version: Ta
322321
avro_entry = next(it)
323322

324323
if format_version == 1:
325-
data_file_v1 = DataFile.from_args(**data_file_dict, _table_format_version=format_version)
324+
data_file_v1 = DataFile.from_args(
325+
_table_format_version=format_version,
326+
block_size_in_bytes=DEFAULT_BLOCK_SIZE,
327+
**common_data_file_args, # type: ignore
328+
)
326329

327330
assert avro_entry == ManifestEntry.from_args(
328331
status=1,

‎tests/integration/test_rest_manifest.py‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,6 @@ def test_write_sample_manifest(table_test_all_types: Table, compression: AvroCom
9090
test_schema = table_test_all_types.schema()
9191
test_spec = table_test_all_types.spec()
9292
wrapped_data_file_v2_debug = DataFile.from_args(
93-
format_version=2,
9493
content=entry.data_file.content,
9594
file_path=entry.data_file.file_path,
9695
file_format=entry.data_file.file_format,
@@ -112,7 +111,7 @@ def test_write_sample_manifest(table_test_all_types: Table, compression: AvroCom
112111
wrapped_entry_v2 = copy(entry)
113112
wrapped_entry_v2.data_file = wrapped_data_file_v2_debug
114113
wrapped_entry_v2_dict = todict(wrapped_entry_v2, [field.name for field in test_spec.fields])
115-
for field in ("first_row_id", "content_offset", "content_size_in_bytes"):
114+
for field in ("first_row_id", "content_offset", "content_size_in_bytes", "spec_id"):
116115
del wrapped_entry_v2_dict["data_file"][field]
117116

118117
with TemporaryDirectory() as tmpdir:

‎tests/test_typedef.py‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import pytest
1818

1919
from pyiceberg.typedef import FrozenDict, KeyDefaultDict, Record
20+
from pyiceberg.types import IntegerType, NestedField, StructType
2021

2122

2223
def test_setitem_frozendict() -> None:
@@ -47,3 +48,20 @@ def test_record_named_args() -> None:
4748
assert r[2] is True
4849

4950
assert repr(r) == "Record[1, a, True]"
51+
52+
53+
def test_record_bind_rejects_unknown_arguments() -> None:
54+
struct = StructType(NestedField(1, "known", IntegerType()))
55+
56+
with pytest.raises(TypeError, match="Unexpected Record fields: unknown"):
57+
Record._bind(struct, known=1, unknown=2)
58+
59+
60+
def test_record_bind_rejects_non_schema_property() -> None:
61+
class RecordWithProperty(Record):
62+
@property
63+
def computed(self) -> int:
64+
return 1
65+
66+
with pytest.raises(TypeError, match="Unexpected RecordWithProperty fields: computed"):
67+
RecordWithProperty._bind(StructType(), computed=1)

0 commit comments

Comments
 (0)