Skip to content

Add missing referenced_data_file field to v2 manifests #3953

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
geruh merged 1 commit into from
Sep 12, 2026
+42 −2
Merged

Add missing referenced_data_file field to v2 manifests #3953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions pyiceberg/manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,13 @@ def __repr__(self) -> str:
required=False,
doc="ID representing sort order for this file",
),
NestedField(
field_id=143,
name="referenced_data_file",
field_type=StringType(),
required=False,
Comment on lines +309 to +313
doc="Fully qualified location (URI with FS scheme) of a data file that all deletes reference",
),
),
3: StructType(
NestedField(
Expand Down
35 changes: 34 additions & 1 deletion tests/avro/test_file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ def test_write_manifest_entry_with_iceberg_read_with_fastavro_v2() -> None:
with avro.AvroOutputFile[ManifestEntry](
output_file=PyArrowFileIO().new_output(tmp_avro_file),
file_schema=MANIFEST_ENTRY_SCHEMAS[2],
record_schema=MANIFEST_ENTRY_SCHEMAS[3],
schema_name="manifest_entry",
metadata=additional_metadata,
) as out:
Expand All @@ -225,12 +226,44 @@ def test_write_manifest_entry_with_iceberg_read_with_fastavro_v2() -> None:
fa_entry = next(it)

v2_entry = todict(entry)
for field in ("first_row_id", "referenced_data_file", "content_offset", "content_size_in_bytes"):
for field in ("first_row_id", "content_offset", "content_size_in_bytes"):
Comment thread
kevinjqliu marked this conversation as resolved.
del v2_entry["data_file"][field]

assert v2_entry == fa_entry


def test_write_v2_referenced_data_file_with_fastavro() -> None:
referenced_data_file = "s3://some-path/data-file.parquet"
entry = ManifestEntry.from_args(
status=ManifestEntryStatus.ADDED,
snapshot_id=25,
data_file=DataFile.from_args(
content=DataFileContent.POSITION_DELETES,
file_path="s3://some-path/delete-file.parquet",
file_format=FileFormat.PARQUET,
partition=Record(),
record_count=3,
file_size_in_bytes=47,
referenced_data_file=referenced_data_file,
),
)

with TemporaryDirectory() as tmpdir:
tmp_avro_file = tmpdir + "/manifest_entry.avro"
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 •
edited
Loading

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.

iceberg-python/pyiceberg/manifest.py

Lines 539 to 541 in 562d3af

@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

@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.

Tracking in #3957

schema_name="manifest_entry",
) as out:
out.write_block([entry])

with open(tmp_avro_file, "rb") as fo:
fa_entry = next(reader(fo))

assert fa_entry["data_file"]["referenced_data_file"] == referenced_data_file


@pytest.mark.parametrize("format_version", [1, 2])
def test_write_manifest_entry_with_fastavro_read_with_iceberg(format_version: TableVersion) -> None:
data_file_dict = {
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/test_rest_manifest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,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", "content_offset", "content_size_in_bytes"):
del wrapped_entry_v2_dict["data_file"][field]

with TemporaryDirectory() as tmpdir:
Expand Down
Loading