mbutrovich commented on code in PR #3354: URL: https://github.com/apache/iceberg-rust/pull/3354#discussion_r4209478253
########## crates/iceberg/src/spec/manifest/_serde.rs: ########## @@ -15,6 +15,14 @@ // specific language governing permissions and limitations // under the License. +//! Serde forms of manifest entries. +//! +//! `Manifest::parse_avro` deserializes these structs from the writer schema +//! without a reader schema, so they decide which manifests read. A field that +//! a writer may omit, per the spec's read rules for every format version the +//! struct reads, must be an `Option` or have `#[serde(default)]`. Review Comment: > This contract claims every format version, but `test_parse_manifest_without_each_field` only builds `manifest_schema_v2`/`ManifestEntryV2`. [`test_parse_v1_manifest_without_each_field`](https://github.com/apache/iceberg-rust/blob/fefcbd6d17a262ce240a6b3ae0ee99164d8f6fed/crates/iceberg/src/spec/manifest/mod.rs#L1757-L1843) does the same for V1. It starts from `manifest_schema_v1` with every field set and removes one field at a time. The 9 fields that v1 marks optional read as their defaults. The 8 required fields, including `snapshot_id`, fail with an error that names the field. [`block_size_in_bytes`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/format/spec.md?plain=1#L726) is required in v1 and absent in v2, so the spec's [read rules](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/format/spec.md?plain=1#L164-L175) say to ignore it. The test checks that a manifest without it reads. Both audits share the loop in [`assert_reads_without_each_field`](https://github.com/apache/iceberg-rust/blob/fefcbd6d17a262ce240a6b3ae0ee99164d8f6fed/crates/iceberg/src/sp ec/manifest/mod.rs#L1604-L1643). > while deciding, worth settling whether a null V1 `snapshot_id` has to be tolerated, since Java can write that for inheritance. The spec marks [`snapshot_id`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/format/spec.md?plain=1#L691) required in v1, and Java writes it as null in v1 when a table sets `compatibility.snapshot-id-inheritance.enabled`. `main` rejects that manifest too. 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). I opened #3371 with a reproduction against `main`. Java and the spec disagree here, so could we settle it in that issue rather than in this PR? -- 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]
