Fix DataFile spec ID and reject unknown record fields by kevinjqliu · Pull Request #3954 · apache/iceberg-python

kevinjqliu · 2026-09-12T01:57:50Z

Summary

This PR fixes two related constructor issues:

DataFile.from_args now sets spec_id when it is provided. spec_id remains transient runtime context and is not included in the serialized data_file record.
Record._bind now raises TypeError for fields that are not part of the selected struct instead of silently dropping them.

Motivation

DataFile.from_args(spec_id=...) previously accepted the argument, but _bind discarded it because spec_id belongs to the containing manifest rather than the serialized data-file schema. The resulting DataFile had no spec context, and accessing spec_id could raise AttributeError.

More generally, silently ignoring unknown fields is a footgun: misspelled, unsupported, or wrong-version arguments appear to work while producing an incomplete record. Validating arguments in _bind makes these mistakes fail immediately for all record types.

Tests

make lint
make test PYTEST_ARGS="-q"
pytest -v -x -m integration tests/integration/test_rest_manifest.py::test_write_sample_manifest

Reject unknown record fields while retaining spec_id as transient DataFile context. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Use the internal table format selector now that unknown DataFile fields are rejected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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>

Keep transient DataFile context out of the expected Avro payload. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

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>

Copilot

🟢 Approval recommended

The focused implementation matches the stated behavior and is adequately tested.

Review details

Files reviewed: 5/5 changed files
Comments generated: 0 new
Review effort level: Balanced

kevinjqliu · 2026-09-12T18:46:13Z

    @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


explicitly add spec_id to the function.
its called in

iceberg-python/pyiceberg/io/pyarrow.py

Line 2771 in 308768d

spec_id=table_metadata.default_spec_id,

iceberg-python/pyiceberg/io/pyarrow.py

Line 2909 in 308768d

spec_id=table_metadata.default_spec_id,

I think this is better. Otherwise caller has to set it after constructing DataFile. For example

data_file = DataFile.from_args() data_file.spec_id = spec_id

kevinjqliu · 2026-09-12T18:46:31Z

+        if spec_id is not None:
+            data_file.spec_id = spec_id
+        return data_file


this is using DataFile's own setter

iceberg-python/pyiceberg/manifest.py

Lines 551 to 560 in 562d3af

# Spec ID should not be stored in the file

_spec_id: int

@property

def spec_id(self) -> int:

return self._spec_id

@spec_id.setter

def spec_id(self, value: int) -> None:

self._spec_id = value

kevinjqliu · 2026-09-12T18:48:11Z


    @classmethod
    def _bind(cls, struct: StructType, **arguments: Any) -> Self:
+        field_names = {field.name for field in struct.fields}


including this fix as part of the PR.

This is the footgun that was silently dropping fields. We now check for unknown fields.

This caught format_version=2, in tests/integration/test_rest_manifest.py which is invalid

kevinjqliu · 2026-09-12T18:48:42Z

        "spec_id": 3,
    }
-    data_file_v2 = DataFile.from_args(**data_file_dict)  # type: ignore
+    data_file = DataFile.from_args(content=DataFileContent.DATA, **common_data_file_args)  # type: ignore


content is a V2 field.

common_data_file_args is shared between v1 and v2 in this test file

kevinjqliu · 2026-09-12T18:49:17Z

-                data_file_v1 = DataFile.from_args(**data_file_dict, _table_format_version=format_version)
+                data_file_v1 = DataFile.from_args(
+                    _table_format_version=format_version,
+                    block_size_in_bytes=DEFAULT_BLOCK_SIZE,


block_size_in_bytes is required in V1.

Resolve the v2 manifest fixture conflict after referenced_data_file was added to the v2 schema. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

kevinjqliu · 2026-09-12T19:02:34Z

    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", "content_offset", "content_size_in_bytes"):
+    for field in ("first_row_id", "content_offset", "content_size_in_bytes", "spec_id"):


spec_id is not serialized so we need to remove it for this comparison to work

kevinjqliu · 2026-09-12T19:03:05Z

    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,


this is not a valid field, the typedef.py change caught it 😄

kevinjqliu · 2026-09-12T19:06:41Z

        "upper_bounds": {1: b"zzzzzzzzzzzzzzzz"},
        "key_metadata": b"\xde\xad\xbe\xef",
        "split_offsets": [4, 133697593],
-        "equality_ids": [],


equality_ids is not a valid V1 field.

common_data_file_args is shared between v1 and v2 in this test file

geruh

lgtm, and aligns with java impl. The spec id is omitted wrt the manifests serialized record.

Fix DataFile spec ID binding

7d6ae63

Reject unknown record fields while retaining spec_id as transient DataFile context. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI lite review requested due to automatic review settings September 12, 2026 01:57

Copilot started reviewing on behalf of kevinjqliu September 12, 2026 01:58 View session

This comment was marked as outdated.

Sign in to view

kevinjqliu marked this pull request as draft September 12, 2026 02:23

kevinjqliu and others added 3 commits September 11, 2026 19:25

Fix DataFile integration fixture version argument

c5c2886

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

b57b2e7

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

1b3a628

Keep transient DataFile context out of the expected Avro payload. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

kevinjqliu changed the title ~~Fix DataFile spec ID binding~~ Fix DataFile spec ID handling Sep 12, 2026

Clarify versioned DataFile test setup

7c5d9ff

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>

kevinjqliu requested a balanced review from Copilot September 12, 2026 18:40

Copilot started reviewing on behalf of kevinjqliu September 12, 2026 18:41 View session

Copilot AI reviewed Sep 12, 2026

View reviewed changes

kevinjqliu commented Sep 12, 2026

View reviewed changes

kevinjqliu changed the title ~~Fix DataFile spec ID handling~~ Fix DataFile spec ID and reject unknown record fields Sep 12, 2026

kevinjqliu requested review from Fokko and geruh September 12, 2026 18:55

kevinjqliu marked this pull request as ready for review September 12, 2026 18:55

Merge main into DataFile spec ID fix

e99c006

Resolve the v2 manifest fixture conflict after referenced_data_file was added to the v2 schema. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

kevinjqliu commented Sep 12, 2026

View reviewed changes

geruh approved these changes Sep 12, 2026

View reviewed changes

geruh added this pull request to the merge queue Sep 12, 2026

Merged via the queue into main with commit 5133f35 Sep 12, 2026
21 checks passed

geruh deleted the kevinjqliu-investigate-datafile-spec-id branch September 12, 2026 21:49

Navigation Menu

Uh oh!

Fix DataFile spec ID and reject unknown record fields - #3954

Fix DataFile spec ID and reject unknown record fields#3954
geruh merged 6 commits into
mainfrom
kevinjqliu-investigate-datafile-spec-id

kevinjqliu commented Sep 12, 2026 •

edited

Loading

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Uh oh!

kevinjqliu Sep 12, 2026

Uh oh!

kevinjqliu Sep 12, 2026

Uh oh!

kevinjqliu Sep 12, 2026

Uh oh!

kevinjqliu Sep 12, 2026

Uh oh!

kevinjqliu Sep 12, 2026

Uh oh!

kevinjqliu Sep 12, 2026

Uh oh!

kevinjqliu Sep 12, 2026

Uh oh!

kevinjqliu Sep 12, 2026

Uh oh!

geruh left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	# Spec ID should not be stored in the file
	_spec_id: int

	@property
	def spec_id(self) -> int:
	return self._spec_id

	@spec_id.setter
	def spec_id(self, value: int) -> None:
	self._spec_id = value

Uh oh!

Conversation

kevinjqliu commented Sep 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Motivation

Tests

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geruh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevinjqliu commented Sep 12, 2026 •

edited

Loading