mbutrovich commented on code in PR #3354: URL: https://github.com/apache/iceberg-rust/pull/3354#discussion_r4198158529
########## crates/iceberg/src/avro/deserializer.rs: ########## @@ -0,0 +1,280 @@ +// 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. +//! +//! apache-avro plans a `SchemaAwareResolvingDeserializer` that resolves against +//! a reader schema (<https://github.com/apache/avro-rs/issues/575>). Once a +//! release includes it, readers can pass their reader schema to +//! `Reader::builder` and drop this module, provided it doesn't reject writer +//! record names that differ from the reader's. The Avro spec requires record +//! names to match, and the writers this crate reads from don't all agree on +//! them. + +use std::fmt; + +use serde::de::value::{ + BorrowedBytesDeserializer, BorrowedStrDeserializer, BytesDeserializer, EnumAccessDeserializer, + MapAccessDeserializer, SeqAccessDeserializer, +}; +use serde::de::{ + DeserializeSeed, Deserializer, EnumAccess, Error, IntoDeserializer, MapAccess, SeqAccess, + Visitor, +}; +use serde::{Deserialize, forward_to_deserialize_any}; + +/// Deserializes the wrapped type through [`ResolvingDeserializer`], for use +/// with `Reader::into_deser_iter`. +pub(crate) struct Resolved<T>(pub T); Review Comment: > `visit_byte_buf`, `visit_seq`/`visit_map`, `visit_enum` and the `visit_borrowed_*` paths aren't hit, and `OptionVisitor` has no `visit_newtype_struct`. I added [`test_resolved_reads_writer_values`](https://github.com/apache/iceberg-rust/blob/91486882b29deb0d97560543e0764aa00a2988fd/crates/iceberg/src/avro/deserializer.rs#L357-L396), [`test_resolved_reads_any_writer_value_as_option`](https://github.com/apache/iceberg-rust/blob/91486882b29deb0d97560543e0764aa00a2988fd/crates/iceberg/src/avro/deserializer.rs#L398-L442), and [`test_resolved_rejects_values_that_do_not_fit`](https://github.com/apache/iceberg-rust/blob/91486882b29deb0d97560543e0764aa00a2988fd/crates/iceberg/src/avro/deserializer.rs#L444-L456). They write each Avro type and read it back through `Resolved`, covering a union, a non-union value read into an `Option`, `int` read into `i64`, `long` read into `i32` in range and out of range, and null read into a non-`Option`. Together they call every visitor method that 0.22's `deserialize_any` can reach: `visit_unit`, `visit_bool`, `visit_i32`, `visit_i64`, `visit_f32`, `visit_f64`, `visit_string`, `visit_byte_buf`, `visit_se q`, `visit_map`, and `visit_enum` ([`deser_schema/mod.rs#L228-L272`](https://github.com/apache/avro-rs/blob/ec5721cb0c80dcde56c1049a004f1d785abd88cf/avro/src/serde/deser_schema/mod.rs#L228-L272)). `deserialize_any` never calls `visit_newtype_struct`, `visit_some`, `visit_none`, or the borrowed and narrow integer methods. That's why `OptionVisitor` has no `visit_newtype_struct`, and the forwarding methods for the others can't be reached from apache-avro. -- 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]
