mbutrovich opened a new issue, #3371: URL: https://github.com/apache/iceberg-rust/issues/3371
### Apache Iceberg Rust version `main` at [`ab047b5`](https://github.com/apache/iceberg-rust/commit/ab047b5e2f6e2ba6b999731653a3f7722b84e6f7) ### Describe the bug `Manifest::parse_avro` returns an error for a V1 manifest entry whose `snapshot_id` is null. Iceberg Java writes these entries for a v1 table that sets [`compatibility.snapshot-id-inheritance.enabled`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/core/src/main/java/org/apache/iceberg/TableProperties.java#L402-L404) to `true`. With that property set, [`FastAppend.appendManifest`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/core/src/main/java/org/apache/iceberg/FastAppend.java#L109-L130) commits a manifest whose entries have no snapshot ID instead of rewriting it, and [`InheritableMetadataFactory`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/core/src/main/java/org/apache/iceberg/InheritableMetadataFactory.java#L63-L67) fills in the ID from the manifest list on read. Java declares `snapshot_id` optional in [`ManifestEntry`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/core/src/main/java/org/apache/iceberg/ManifestEntry.java#L52), and [ `V1Metadata`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/core/src/main/java/org/apache/iceberg/V1Metadata.java#L207-L213) uses the same field for v1. The spec marks [`snapshot_id`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/format/spec.md?plain=1#L691) required in v1 and optional in v2, and describes [inheriting it when null](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/format/spec.md?plain=1#L702) as a v2 addition. As I read it, Java and the spec disagree on whether a v1 entry can have a null `snapshot_id`. iceberg-rust reads a V1 `snapshot_id` as a required `long` in the reader schema ([`entry.rs#L190-L198`](https://github.com/apache/iceberg-rust/blob/ab047b5e2f6e2ba6b999731653a3f7722b84e6f7/crates/iceberg/src/spec/manifest/entry.rs#L190-L198)) and an `i64` in [`ManifestEntryV1`](https://github.com/apache/iceberg-rust/blob/ab047b5e2f6e2ba6b999731653a3f7722b84e6f7/crates/iceberg/src/spec/manifest/_serde.rs#L67-L71). apache-avro 0.21 [takes the null out of the writer's union](https://github.com/apache/avro-rs/blob/04707999f75278fdea927ee8d2a59de41d8f22a7/avro/src/types.rs#L646-L655) and [rejects it as a `long`](https://github.com/apache/avro-rs/blob/04707999f75278fdea927ee8d2a59de41d8f22a7/avro/src/types.rs#L890-L895). [`ManifestEntry::inherit_data`](https://github.com/apache/iceberg-rust/blob/ab047b5e2f6e2ba6b999731653a3f7722b84e6f7/crates/iceberg/src/spec/manifest/entry.rs#L107-L110) already fills a missing `snapshot_id` from the manifest list's `added_snapshot_id`, but a V1 entry n ever reaches it with `None`. ### To Reproduce This program writes a V1 manifest for an unpartitioned table with the entry schema Java uses, and one entry with a null `snapshot_id`. `Cargo.toml`: ```toml [package] name = "v1-null-snapshot-id" version = "0.0.0" edition = "2024" publish = false [dependencies] apache-avro = "0.21" iceberg = { git = "https://github.com/apache/iceberg-rust";, rev = "ab047b5e2f6e2ba6b999731653a3f7722b84e6f7" } [workspace] ``` `src/main.rs`: ```rust use apache_avro::types::{Record, Value}; use apache_avro::{Schema, Writer}; use iceberg::spec::Manifest; /// The V1 manifest entry schema as Iceberg Java writes it for an unpartitioned /// table: `snapshot_id` is optional, and `data_file` has only its required fields. const ENTRY_SCHEMA: &str = r#"{ "type": "record", "name": "manifest_entry", "fields": [ {"name": "status", "type": "int", "field-id": 0}, {"name": "snapshot_id", "type": ["null", "long"], "default": null, "field-id": 1}, {"name": "data_file", "field-id": 2, "type": {"type": "record", "name": "r2", "fields": [ {"name": "file_path", "type": "string", "field-id": 100}, {"name": "file_format", "type": "string", "field-id": 101}, {"name": "partition", "field-id": 102, "type": {"type": "record", "name": "r102", "fields": []}}, {"name": "record_count", "type": "long", "field-id": 103}, {"name": "file_size_in_bytes", "type": "long", "field-id": 104}, {"name": "block_size_in_bytes", "type": "long", "field-id": 105} ]}} ] }"#; fn main() { let schema = Schema::parse_str(ENTRY_SCHEMA).unwrap(); let mut writer = Writer::new(&schema, Vec::new()); for (key, value) in [ ("schema", r#"{"type":"struct","schema-id":0,"fields":[{"id":1,"name":"id","required":false,"type":"long"}]}"#), ("schema-id", "0"), ("partition-spec", "[]"), ("partition-spec-id", "0"), ("format-version", "1"), ] { writer.add_user_metadata(key.to_string(), value).unwrap(); } let data_file = Value::Record(vec![ ("file_path".into(), "s3://bucket/table/data/a.parquet".into()), ("file_format".into(), "PARQUET".into()), ("partition".into(), Value::Record(vec![])), ("record_count".into(), 10i64.into()), ("file_size_in_bytes".into(), 100i64.into()), ("block_size_in_bytes".into(), 67108864i64.into()), ]); let mut entry = Record::new(&schema).unwrap(); entry.put("status", 1i32); // Java writes a null snapshot ID when the table enables snapshot ID inheritance. entry.put("snapshot_id", Value::Union(0, Box::new(Value::Null))); entry.put("data_file", data_file); writer.append(entry).unwrap(); let bytes = writer.into_inner().unwrap(); match Manifest::parse_avro(&bytes) { Ok(manifest) => println!("read {:?}", manifest.entries()[0].snapshot_id()), Err(err) => println!("error: {err}"), } } ``` `cargo run` prints: ``` error: DataInvalid => Failure in conversion with avro, source: Expected Value::Long or Value::Int, got: Null ``` With `snapshot_id` set to `Value::Union(1, Box::new(Value::Long(7)))`, it prints `read Some(7)`. The reader in #3354 rejects the null value too, with `invalid type: unit value, expected i64`. ### Expected behavior Should iceberg-rust read these manifests? If so, `ManifestEntryV1` could read `snapshot_id` as an `Option<i64>` and let `inherit_data` fill it in from the manifest list, as it does for V2. Because the spec requires the field in v1, I'm not sure whether this belongs in iceberg-rust or should first be raised as a spec clarification on the dev list. ### Willingness to contribute I cannot contribute a fix for this bug at this time -- 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]
