mfellner opened a new issue, #3399: URL: https://github.com/apache/iceberg-rust/issues/3399
Related to #2411 (v3 tracking). ### Apache Iceberg Rust version main at 2fddb1601 ### Describe the bug The Avro writer schema for v2 manifests declares three `data_file` fields that the spec defines only for v3: - `142 first_row_id` - `144 content_offset` - `145 content_size_in_bytes` In the spec's data file table, these three fields have no v2 entry. `143 referenced_data_file` is optional in both v2 and v3 and is not part of this issue. Code on 2fddb1601: - [`data_file_fields_v2`](https://github.com/apache/iceberg-rust/blob/2fddb1601a9af1c97a559baa9865e1494d93756c/crates/iceberg/src/spec/manifest/entry.rs#L545-L574) includes all three fields. The comment at [L569-L570](https://github.com/apache/iceberg-rust/blob/2fddb1601a9af1c97a559baa9865e1494d93756c/crates/iceberg/src/spec/manifest/entry.rs#L569-L570) already questions the two content fields in the v2 schema. - [`write_manifest_file`](https://github.com/apache/iceberg-rust/blob/2fddb1601a9af1c97a559baa9865e1494d93756c/crates/iceberg/src/spec/manifest/writer.rs#L431-L433) uses `manifest_schema_v2` for both `FormatVersion::V2` and `FormatVersion::V3`. - [`ManifestWriter::add_file`](https://github.com/apache/iceberg-rust/blob/2fddb1601a9af1c97a559baa9865e1494d93756c/crates/iceberg/src/spec/manifest/writer.rs#L302) is public. [`check_data_file`](https://github.com/apache/iceberg-rust/blob/2fddb1601a9af1c97a559baa9865e1494d93756c/crates/iceberg/src/spec/manifest/writer.rs#L252-L276) checks only the content type, not the format version. The `DataFileBuilder` setters for these three fields are public and do not depend on the format version (`crates/iceberg/public-api.txt`). A v2 manifest can therefore carry v3 values, and `Manifest::parse_avro` returns them. Java's v2 writer uses a different schema. In [`V2Metadata.fileType`](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/V2Metadata.java#L259-L279), the v2 data file fields exclude 142, 144 and 145 and keep 143. [`ManifestWriter`](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/ManifestWriter.java#L469) uses `V2Metadata.entrySchema` for v2 writers (also at L511). This matches the gap described in [this comment on #2411](https://github.com/apache/iceberg-rust/issues/2411#issuecomment-6058105246): "V3 fields being written into V1+V2 manifests". Scope: - v1 manifests are not affected. The v1 schema has none of the three fields, and the v1 writer drops the values: they parse back as `None`. - Manifest lists are not affected, by code reading only. `V2_SCHEMA` in `crates/iceberg/src/spec/manifest_list/_const_schema.rs` has no `first_row_id`. The #2411 checklist item "Gate v3-only metadata fields when reading v1/v2" covers the read side. This issue covers what the writer emits. ### To Reproduce On 2fddb1601, add the following test to the `tests` module in `crates/iceberg/src/spec/manifest/writer.rs` and run `cargo test -p iceberg --lib -- test_v2_manifest_schema_has_only_v2_data_file_fields`. ```rust fn data_file_field_ids(bytes: &[u8]) -> Vec<i64> { let reader = apache_avro::Reader::new(bytes).unwrap(); let apache_avro::Schema::Record(entry) = reader.writer_schema() else { panic!() }; let field = entry.fields.iter().find(|f| f.name == "data_file").unwrap(); let apache_avro::Schema::Record(data_file) = &field.schema else { panic!() }; data_file.fields.iter() .map(|f| f.custom_attributes["field-id"].as_i64().unwrap()) .collect() } #[tokio::test] async fn test_v2_manifest_schema_has_only_v2_data_file_fields() { let schema = Arc::new(Schema::builder() .with_fields(vec![Arc::new(NestedField::optional(1, "id", Type::Primitive(PrimitiveType::Long)))]) .build().unwrap()); let partition_spec = PartitionSpec::builder(schema.clone()).with_spec_id(0).build().unwrap(); let data_file = DataFileBuilder::default() .content(DataContentType::Data) .file_path("s3://bucket/table/data/a.parquet".to_string()) .file_format(DataFileFormat::Parquet) .record_count(1) .file_size_in_bytes(10) .first_row_id(Some(1000)) .content_offset(Some(4)) .content_size_in_bytes(Some(8)) .build().unwrap(); let tmp_dir = TempDir::new().unwrap(); let path = tmp_dir.path().join("v2_manifest.avro"); let output_file = FileIO::new_with_fs().new_output(path.to_str().unwrap()).unwrap(); let mut writer = ManifestWriterBuilder::new(output_file, Some(1), schema, partition_spec) .build_v2_data(); writer.add_file(data_file, 1).unwrap(); writer.write_manifest_file().await.unwrap(); let bytes = fs::read(&path).unwrap(); let expected = vec![134, 100, 101, 102, 103, 104, 108, 109, 110, 137, 125, 128, 131, 132, 135, 140, 143]; assert_eq!(data_file_field_ids(&bytes), expected); } ``` Output on 2fddb1601: ``` assertion `left == right` failed left: [134, 100, 101, 102, 103, 104, 108, 109, 110, 137, 125, 128, 131, 132, 135, 140, 142, 143, 144, 145] right: [134, 100, 101, 102, 103, 104, 108, 109, 110, 137, 125, 128, 131, 132, 135, 140, 143] ``` The expected ids are the v2 `data_file` field ids in the spec's table. The same v2 bytes, parsed with `Manifest::parse_avro`, return `first_row_id = Some(1000)`, `content_offset = Some(4)` and `content_size_in_bytes = Some(8)`. I checked this with a temporary probe test in the same tree. ### Expected behavior - The `data_file` record in a v2 manifest's Avro schema contains only the v2 field ids, with no 142, 144 or 145. - v3-only values set on a `DataFile` are not written into v2 manifests. Rejecting them or omitting them are both reasonable; I leave that choice to maintainers. Impact: the written v2 schema differs from the spec. I did not test read-side behaviour on a real table or with other implementations. ### Willingness to contribute I would be willing to contribute a fix with guidance from the Iceberg community (any contribution would be agent-assisted and I would disclose that). Found during an agent-assisted audit of v3 support (see #2411). The reproduction above was run against main at 2fddb1601. -- 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]
