From 7d6ae630a6e1dfaf04c38dfba1fe06f22152cab3 Mon Sep 17 00:00:00 2001 From: Kevin Liu Date: Fri, 11 Sep 2026 18:57:34 -0700 Subject: [PATCH 1/5] 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> --- pyiceberg/manifest.py | 9 +++++++-- pyiceberg/typedef.py | 3 +++ tests/avro/test_file.py | 13 ++++++++----- tests/test_typedef.py | 18 ++++++++++++++++++ 4 files changed, 36 insertions(+), 7 deletions(-) diff --git a/pyiceberg/manifest.py b/pyiceberg/manifest.py index 2b54fff509..d1203dae45 100644 --- a/pyiceberg/manifest.py +++ b/pyiceberg/manifest.py @@ -464,9 +464,14 @@ def data_file_with_partition(partition_type: StructType, format_version: TableVe class DataFile(Record): @classmethod - def from_args(cls, _table_format_version: TableVersion = DEFAULT_READ_VERSION, **arguments: Any) -> DataFile: + def from_args( + cls, _table_format_version: TableVersion = DEFAULT_READ_VERSION, *, spec_id: int | None = None, **arguments: Any + ) -> DataFile: struct = DATA_FILE_TYPE[_table_format_version] - return super()._bind(struct, **arguments) + data_file = super()._bind(struct, **arguments) + if spec_id is not None: + data_file.spec_id = spec_id + return data_file @property def content(self) -> DataFileContent: diff --git a/pyiceberg/typedef.py b/pyiceberg/typedef.py index 6989144ef9..489a44d5f7 100644 --- a/pyiceberg/typedef.py +++ b/pyiceberg/typedef.py @@ -179,6 +179,9 @@ class Record(StructProtocol): @classmethod def _bind(cls, struct: StructType, **arguments: Any) -> Self: + field_names = {field.name for field in struct.fields} + if unknown_fields := arguments.keys() - field_names: + raise TypeError(f"Unexpected {cls.__name__} fields: {', '.join(sorted(unknown_fields))}") return cls(*[arguments[field.name] if field.name in arguments else field.initial_default for field in struct.fields]) def __init__(self, *data: Any) -> None: diff --git a/tests/avro/test_file.py b/tests/avro/test_file.py index 7bd0422439..cd1b6151d2 100644 --- a/tests/avro/test_file.py +++ b/tests/avro/test_file.py @@ -233,8 +233,7 @@ def test_write_manifest_entry_with_iceberg_read_with_fastavro_v2() -> None: @pytest.mark.parametrize("format_version", [1, 2]) def test_write_manifest_entry_with_fastavro_read_with_iceberg(format_version: TableVersion) -> None: - data_file_dict = { - "content": DataFileContent.DATA, + common_data_file_args = { "file_path": "s3://some-path/some-file.parquet", "file_format": FileFormat.PARQUET, "partition": Record(), @@ -248,11 +247,11 @@ def test_write_manifest_entry_with_fastavro_read_with_iceberg(format_version: Ta "upper_bounds": {1: b"zzzzzzzzzzzzzzzz"}, "key_metadata": b"\xde\xad\xbe\xef", "split_offsets": [4, 133697593], - "equality_ids": [], "sort_order_id": 4, "spec_id": 3, } - data_file_v2 = DataFile.from_args(**data_file_dict) # type: ignore + data_file_v2 = DataFile.from_args(content=DataFileContent.DATA, **common_data_file_args) # type: ignore + assert data_file_v2.spec_id == 3 entry = ManifestEntry.from_args( status=ManifestEntryStatus.ADDED, @@ -289,7 +288,11 @@ def test_write_manifest_entry_with_fastavro_read_with_iceberg(format_version: Ta avro_entry = next(it) if format_version == 1: - data_file_v1 = DataFile.from_args(**data_file_dict, _table_format_version=format_version) + data_file_v1 = DataFile.from_args( + block_size_in_bytes=DEFAULT_BLOCK_SIZE, + _table_format_version=format_version, + **common_data_file_args, # type: ignore + ) assert avro_entry == ManifestEntry.from_args( status=1, diff --git a/tests/test_typedef.py b/tests/test_typedef.py index fbbb619968..f13349a42f 100644 --- a/tests/test_typedef.py +++ b/tests/test_typedef.py @@ -17,6 +17,7 @@ import pytest from pyiceberg.typedef import FrozenDict, KeyDefaultDict, Record +from pyiceberg.types import IntegerType, NestedField, StructType def test_setitem_frozendict() -> None: @@ -47,3 +48,20 @@ def test_record_named_args() -> None: assert r[2] is True assert repr(r) == "Record[1, a, True]" + + +def test_record_bind_rejects_unknown_arguments() -> None: + struct = StructType(NestedField(1, "known", IntegerType())) + + with pytest.raises(TypeError, match="Unexpected Record fields: unknown"): + Record._bind(struct, known=1, unknown=2) + + +def test_record_bind_rejects_non_schema_property() -> None: + class RecordWithProperty(Record): + @property + def computed(self) -> int: + return 1 + + with pytest.raises(TypeError, match="Unexpected RecordWithProperty fields: computed"): + RecordWithProperty._bind(StructType(), computed=1) From c5c28860aa8b159c859cdb9bb7fc0c17082e2dc9 Mon Sep 17 00:00:00 2001 From: Kevin Liu Date: Fri, 11 Sep 2026 19:25:36 -0700 Subject: [PATCH 2/5] 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> --- tests/integration/test_rest_manifest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_rest_manifest.py b/tests/integration/test_rest_manifest.py index 5ca9f22a8c..d8d6c2a03e 100644 --- a/tests/integration/test_rest_manifest.py +++ b/tests/integration/test_rest_manifest.py @@ -90,7 +90,7 @@ def test_write_sample_manifest(table_test_all_types: Table, compression: AvroCom test_schema = table_test_all_types.schema() test_spec = table_test_all_types.spec() wrapped_data_file_v2_debug = DataFile.from_args( - format_version=2, + _table_format_version=2, content=entry.data_file.content, file_path=entry.data_file.file_path, file_format=entry.data_file.file_format, From b57b2e70b354e504f021729eca2495a3a3969432 Mon Sep 17 00:00:00 2001 From: Kevin Liu Date: Sat, 12 Sep 2026 09:48:37 -0700 Subject: [PATCH 3/5] 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> --- tests/integration/test_rest_manifest.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/integration/test_rest_manifest.py b/tests/integration/test_rest_manifest.py index d8d6c2a03e..a54ac34f32 100644 --- a/tests/integration/test_rest_manifest.py +++ b/tests/integration/test_rest_manifest.py @@ -90,7 +90,6 @@ def test_write_sample_manifest(table_test_all_types: Table, compression: AvroCom test_schema = table_test_all_types.schema() test_spec = table_test_all_types.spec() wrapped_data_file_v2_debug = DataFile.from_args( - _table_format_version=2, content=entry.data_file.content, file_path=entry.data_file.file_path, file_format=entry.data_file.file_format, From 1b3a628c4ab1a3d30b7e324eee3e901a32d2d116 Mon Sep 17 00:00:00 2001 From: Kevin Liu Date: Sat, 12 Sep 2026 11:36:54 -0700 Subject: [PATCH 4/5] 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> --- tests/integration/test_rest_manifest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_rest_manifest.py b/tests/integration/test_rest_manifest.py index a54ac34f32..2b4dc3fcac 100644 --- a/tests/integration/test_rest_manifest.py +++ b/tests/integration/test_rest_manifest.py @@ -111,7 +111,7 @@ def test_write_sample_manifest(table_test_all_types: Table, compression: AvroCom wrapped_entry_v2 = copy(entry) wrapped_entry_v2.data_file = wrapped_data_file_v2_debug wrapped_entry_v2_dict = todict(wrapped_entry_v2, [field.name for field in test_spec.fields]) - for field in ("first_row_id", "referenced_data_file", "content_offset", "content_size_in_bytes"): + for field in ("first_row_id", "referenced_data_file", "content_offset", "content_size_in_bytes", "spec_id"): del wrapped_entry_v2_dict["data_file"][field] with TemporaryDirectory() as tmpdir: From 7c5d9ff9a767a28bd5ee01e2c3aa78ebd6909e97 Mon Sep 17 00:00:00 2001 From: Kevin Liu Date: Sat, 12 Sep 2026 11:38:26 -0700 Subject: [PATCH 5/5] 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> --- tests/avro/test_file.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/avro/test_file.py b/tests/avro/test_file.py index cd1b6151d2..9350e89452 100644 --- a/tests/avro/test_file.py +++ b/tests/avro/test_file.py @@ -250,13 +250,13 @@ def test_write_manifest_entry_with_fastavro_read_with_iceberg(format_version: Ta "sort_order_id": 4, "spec_id": 3, } - data_file_v2 = DataFile.from_args(content=DataFileContent.DATA, **common_data_file_args) # type: ignore - assert data_file_v2.spec_id == 3 + data_file = DataFile.from_args(content=DataFileContent.DATA, **common_data_file_args) # type: ignore + assert data_file.spec_id == 3 entry = ManifestEntry.from_args( status=ManifestEntryStatus.ADDED, snapshot_id=8638475580105682862, - data_file=data_file_v2, + data_file=data_file, ) with TemporaryDirectory() as tmpdir: @@ -289,8 +289,8 @@ def test_write_manifest_entry_with_fastavro_read_with_iceberg(format_version: Ta if format_version == 1: data_file_v1 = DataFile.from_args( - block_size_in_bytes=DEFAULT_BLOCK_SIZE, _table_format_version=format_version, + block_size_in_bytes=DEFAULT_BLOCK_SIZE, **common_data_file_args, # type: ignore )