Skip to content

Add missing referenced_data_file field to v2 manifests - #3953

Merged
geruh merged 1 commit into
apache:mainfrom
kevinjqliu:kevinjqliu-add-v2-referenced-data-file
Sep 12, 2026
Merged

Add missing referenced_data_file field to v2 manifests#3953
geruh merged 1 commit into
apache:mainfrom
kevinjqliu:kevinjqliu-add-v2-referenced-data-file

Conversation

@kevinjqliu

@kevinjqliu kevinjqliu commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

The Iceberg spec allows v2 position-delete files to use referenced_data_file (field ID 143) when all deletes reference a single data file. PyIceberg currently includes this field in its v3 data-file schema, but not v2.

This PR adds the optional string field to the v2 schema, updates the existing Avro and REST manifest comparisons to retain it, and adds a focused non-null serialization test. The Avro tests explicitly declare the v3 in-memory record schema while writing v2 files. No production record-layout or helper changes are included.

Related: #3690 introduced the v3 manifest read projection.

Copilot AI lite review requested due to automatic review settings September 12, 2026 01:53
@kevinjqliu
kevinjqliu requested review from Fokko and geruh September 12, 2026 01:55

This comment was marked as outdated.

@kevinjqliu
kevinjqliu marked this pull request as draft September 12, 2026 02:24
@kevinjqliu
kevinjqliu force-pushed the kevinjqliu-add-v2-referenced-data-file branch from 992d0ce to 9ce1ab6 Compare September 12, 2026 16:48
@kevinjqliu kevinjqliu changed the title Fix v2 referenced data file schema Add missing referenced_data_file field to v2 manifests Sep 12, 2026
@kevinjqliu
kevinjqliu requested a balanced review from Copilot September 12, 2026 17:14
@kevinjqliu
kevinjqliu marked this pull request as ready for review September 12, 2026 17:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The tests do not verify a non-null value and conceal a positional writer mismatch that drops it.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread tests/avro/test_file.py
Add optional referenced_data_file (field 143) to v2 manifests, retain it in existing expectations, and cover a non-null reference using explicit canonical-record projection.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@kevinjqliu
kevinjqliu force-pushed the kevinjqliu-add-v2-referenced-data-file branch from 9ce1ab6 to fa2dafb Compare September 12, 2026 17:48
@kevinjqliu
kevinjqliu requested a balanced review from Copilot September 12, 2026 17:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The schema change matches the specification and is covered by focused serialization and integration assertions.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@geruh geruh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@geruh
geruh enabled auto-merge September 12, 2026 17:59
Comment thread tests/avro/test_file.py
with avro.AvroOutputFile[ManifestEntry](
output_file=PyArrowFileIO().new_output(tmp_avro_file),
file_schema=MANIFEST_ENTRY_SCHEMAS[2],
record_schema=MANIFEST_ENTRY_SCHEMAS[3],

@kevinjqliu kevinjqliu Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to declare the record schema as v3 here, even though the write schema is v2. Otherwise, referenced_data_file gets written as null. 😭

this is because DataFile.from_args() defaults to v3, but the writer uses position-based indexes unless we provide the record schema.

In V2 schema, referenced_data_file should be index 16 because first_row_id doesnt exist in V2.
In V3 schema, referenced_data_file is index 17.

Without record_schema, the writer reads index 16 (first_row_id) instead of 17. Declaring v3 lets the existing field-ID projection handle this mapping.

The getter also assumes index 17 and I dont want to add an if branch to the getter.

@property
def referenced_data_file(self) -> str | None:
return self._data[17]

This is an existing problem so I think the best way to resolve it is to have records retain their schema and use field IDs for access. On read, we should infer the schema from the file. On write, we should specify the version and let manifest IO handle the conversion.

We can do this as a follow up

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking in #3957

@geruh
geruh added this pull request to the merge queue Sep 12, 2026
Merged via the queue into apache:main with commit 308768d Sep 12, 2026
21 checks passed
@kevinjqliu
kevinjqliu deleted the kevinjqliu-add-v2-referenced-data-file branch September 12, 2026 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants