kevinjqliu commented on code in PR #3953:
URL: https://github.com/apache/iceberg-python/pull/3953#discussion_r3997076081
##########
tests/avro/test_file.py:
##########
@@ -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"):
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],
Review Comment:
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.
https://github.com/apache/iceberg-python/blob/562d3af3a77476bcd7b923f31da60200c1b2563c/pyiceberg/manifest.py#L539-L541
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
