mbutrovich commented on code in PR #3354:
URL: https://github.com/apache/iceberg-rust/pull/3354#discussion_r4198186746
##########
crates/iceberg/src/spec/manifest/mod.rs:
##########
@@ -61,43 +84,35 @@ impl Manifest {
let partition_struct_type = Type::Struct(partition_type.clone());
let entries = match metadata.format_version {
- FormatVersion::V1 => {
- let schema = manifest_schema_v1(&partition_type)?;
- let reader = AvroReader::with_schema(&schema, bs)?;
- reader
- .into_iter()
- .map(|value| {
-
from_value::<_serde::ManifestEntryV1>(&value?)?.try_into(
- metadata.partition_spec.spec_id(),
- &partition_struct_type,
- &metadata.schema,
- )
- })
- .collect::<Result<Vec<_>>>()?
- }
+ FormatVersion::V1 => reader
+ .into_deser_iter::<Resolved<_serde::ManifestEntryV1>>()
+ .map(|entry| {
+ entry?.0.try_into(
+ metadata.partition_spec.spec_id(),
+ &partition_struct_type,
+ &metadata.schema,
+ )
+ })
+ .collect::<Result<Vec<_>>>()?,
// Manifest Schema & Manifest Entry did not change between V2 and
V3
- FormatVersion::V2 | FormatVersion::V3 => {
- let schema = manifest_schema_v2(&partition_type)?;
- let reader = AvroReader::with_schema(&schema, bs)?;
- reader
- .into_iter()
- .map(|value| {
-
from_value::<_serde::ManifestEntryV2>(&value?)?.try_into(
- metadata.partition_spec.spec_id(),
- &partition_struct_type,
- &metadata.schema,
- )
- })
- .collect::<Result<Vec<_>>>()?
- }
+ FormatVersion::V2 | FormatVersion::V3 => reader
+ .into_deser_iter::<Resolved<_serde::ManifestEntryV2>>()
Review Comment:
> I'd want this written into the module doc
The [`_serde` module
doc](https://github.com/apache/iceberg-rust/blob/91486882b29deb0d97560543e0764aa00a2988fd/crates/iceberg/src/spec/manifest/_serde.rs#L18-L24)
now says that the structs are the compatibility contract and that a field a
writer may omit must be an `Option` or have `#[serde(default)]`.
> backed by a table-driven test asserting every spec-optional/defaulted
field is tolerated when absent, plus at least one real Java/PyIceberg-written
manifest in the fixtures rather than only synthetic JSON through this crate's
own writer.
[`test_parse_manifest_without_each_field`](https://github.com/apache/iceberg-rust/blob/91486882b29deb0d97560543e0764aa00a2988fd/crates/iceberg/src/spec/manifest/mod.rs#L1597-L1728)
starts from the manifest schema that iceberg-rust writes, with every field
set, and removes one field at a time from both the writer schema and the entry.
For each of the 18 fields that the spec's [read
rules](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/format/spec.md?plain=1#L164-L175)
let a writer omit, it checks that the entry reads with that field at its
default. `content` reads as data (0), the metrics maps read as empty, and the
rest read as `None`. For each of the 7 required fields, it checks that the read
fails with an error naming the field. Removing `#[serde(default)]` from
`content` makes it fail.
[`test_parse_manifest_written_by_pyiceberg`](https://github.com/apache/iceberg-rust/blob/91486882b29deb0d97560543e0764aa00a2988fd/crates/iceberg/src/spec/manifest
/mod.rs#L2016-L2102) reads
[`pyiceberg-v2-data.avro`](https://github.com/apache/iceberg-rust/blob/91486882b29deb0d97560543e0764aa00a2988fd/crates/iceberg/testdata/manifests/pyiceberg-v2-data.avro),
which PyIceberg 0.12.0 wrote with its own Avro schema, and compares the whole
manifest.
--
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]
