Fix DataFile spec ID binding - #3954
Draft
kevinjqliu wants to merge 2 commits into
Draft
Conversation
Reject unknown record fields while retaining spec_id as transient DataFile context. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Strict binding breaks an existing integration caller that passes format_version instead of the supported parameter.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes transient DataFile.spec_id handling and enforces schema-based validation for record binding.
Changes:
- Preserves transient
spec_idvalues. - Rejects unknown fields during binding.
- Adds regression tests for binding and format handling.
File summaries
| File | Summary |
|---|---|
tests/test_typedef.py |
Adds tests for unknown-field rejection. |
tests/avro/test_file.py |
Tests spec_id preservation and format handling. |
pyiceberg/typedef.py |
Adds schema validation. Critical: an existing caller passes unsupported format_version, causing TypeError. |
pyiceberg/manifest.py |
Preserves transient spec_id. Critical: the existing format_version caller must be updated or supported via an intentional alias. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+468
to
+471
| 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) |
Comment on lines
+182
to
+184
| 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))}") |
kevinjqliu
marked this pull request as draft
September 12, 2026 02:23
Use the internal table format selector now that unknown DataFile fields are rejected. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
DataFile.from_args(spec_id=...)passedspec_idthrough**arguments, butRecord._bindonly retained fields in the serializeddata_filestruct. Becausespec_idbelongs to the containing manifest rather than the data-file schema, it was silently discarded and later access could raiseAttributeError.Changes
spec_idexplicitly inDataFile.from_argsand store it as transient runtime context, keeping it out of Avro serialization.Record._bindreject unknown arguments instead of silently dropping them, preventing similar bugs.This keeps cross-version compatibility in schema resolution while making construction and writing follow the target Iceberg format version.
Tests
make lint