laskoviymishka commented on code in PR #3354: URL: https://github.com/apache/iceberg-rust/pull/3354#discussion_r4204959657
########## crates/iceberg/src/avro/deserializer.rs: ########## @@ -0,0 +1,457 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Deserialization straight from an Avro writer schema, with the parts of Avro +//! schema resolution that apache-avro's schema-aware deserializer leaves out. +//! +//! `Reader::into_deser_iter` decodes into serde types without building +//! `apache_avro::types::Value`s, but it requires the reader schema to equal the +//! writer schema and applies no resolution rules. [`ResolvingDeserializer`] +//! wraps it and routes every value through `deserialize_any`, which follows the +//! writer schema. As a result: +//! +//! - Avro record names don't have to match serde type names. +//! - A writer value that isn't a union reads into an `Option`. +//! - A writer union reads into a type that isn't an `Option`. A null value +//! returns an error. +//! - serde's numeric visitors convert between numeric types, so an `int` reads +//! into an `i64` and a `float` into an `f64`. An integer that doesn't fit the +//! target, such as a `long` above `i32::MAX` read into an `i32`, returns an +//! error. Conversions into `f32` or `f64` use `as` and can lose precision. Review Comment: This doc lists what the adapter does but not what it drops versus the old resolving reader — aliases, reader-schema defaults, string-to-bytes promotion, name/order matching outside the partition record. That gap bites because a type mismatch the old path rejected up front now only surfaces as a serde error when a value is actually hit, so a mismatched column whose values are all null reads clean. I'd add the "doesn't support" list here, and — the real guard — a differential test running the fixtures plus a few synthetic variants through both the 0.22 `with_schema` reader and `Resolved`, asserting equal results, so any divergence is intentional rather than something we eyeball. ########## crates/iceberg/src/spec/values/serde.rs: ########## @@ -44,6 +46,53 @@ pub(crate) mod _serde { pub fn try_into(self, ty: &Type) -> Result<Option<Literal>, Error> { self.0.try_into(ty) } + + /// Matches the fields of a record to `struct_type` by name, the way Avro + /// schema resolution matches record fields. The result has + /// `struct_type`'s fields in its order, with null for an optional field the + /// record lacks. Record fields that `struct_type` lacks are dropped. Values + /// other than records are returned unchanged. + /// + /// Fields aren't matched by field ID. A writer that stores a field under + /// another name, such as Java replacing characters that Avro names don't + /// allow, reads as a missing field. + pub fn project_by_name(self, struct_type: &StructType) -> Result<Self, Error> { Review Comment: The caveat here is accurate, but the consequence is sharper than it reads: partition fields are all optional, so if the writer's names don't line up at all — Java sanitizes `ts.day` to `ts_x2Eday` — every field misses and reads as Null with no error, and this is now the sole safeguard since we dropped Avro's own resolution. All-null partition values corrupt pruning and nothing downstream can tell. I'd error when the record has fields but none of them match any spec field, or sanitize `field.name` the way Java's `makeCompatibleName` does before the lookup. ########## crates/iceberg/src/spec/manifest/mod.rs: ########## @@ -45,9 +52,40 @@ pub struct Manifest { } impl Manifest { - /// Parse manifest metadata and entries from bytes of avro file. - pub(crate) fn try_from_avro_bytes(bs: &[u8]) -> Result<(ManifestMetadata, Vec<ManifestEntry>)> { - let reader = AvroReader::new(bs)?; + /// Parse manifest metadata and entries from bytes of avro file. `location` + /// names the manifest in warnings. + pub(crate) fn try_from_avro_bytes( + bs: &[u8], + location: Option<&str>, + ) -> Result<(ManifestMetadata, Vec<ManifestEntry>)> { + let rewritten; + let reader = match AvroReader::new(bs) { + Ok(reader) => reader, + // iceberg-rust repeated `decimal` definitions before + // `schema_to_avro_schema` defined each named type once, so this + // fallback stays while tables can contain manifests it wrote. + Err(e) if matches!(e.details(), Details::AmbiguousSchemaDefinition(_)) => { + let Ok(Some((bs, repeated))) = define_named_types_once(bs) else { + return Err(e.into()); + }; + let location = location.unwrap_or("<unknown location>"); + if WARNED_REPEATED_DEFINITIONS.swap(true, Ordering::Relaxed) { + tracing::debug!( + "Manifest {location} defines Avro named types {repeated:?} more than once." + ); + } else { + tracing::warn!( + "Manifest {location} defines Avro named types {repeated:?} more than once, \ + which the Avro specification doesn't allow. Reading it with each repeated \ + definition replaced by a reference to the first. Later manifests like \ + this are logged at debug level." + ); + } + rewritten = bs; + AvroReader::new(rewritten.as_slice())? Review Comment: Same error swap we fixed on the first path, one branch down: if the rewrite succeeds but this reparse fails, `?` hands back the secondary error, not the `AmbiguousSchemaDefinition` that sent us into the fallback. I'd fall back to the original — `match AvroReader::new(rewritten.as_slice()) { Ok(r) => r, Err(_) => return Err(e.into()) }` — or attach the secondary as context. ########## 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`. V1 reads a different shape — `snapshot_id` is a plain `i64` there, not `Option`, and the V1-only fields aren't exercised — so a V1 regression ships without this test catching it. I'd either add a V1 variant or narrow the wording to V2/V3; while deciding, worth settling whether a null V1 `snapshot_id` has to be tolerated, since Java can write that for inheritance. -- 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]
