mbutrovich commented on code in PR #3354:
URL: https://github.com/apache/iceberg-rust/pull/3354#discussion_r4198166603
##########
crates/iceberg/src/avro/schema.rs:
##########
@@ -43,6 +43,29 @@ const LOGICAL_TYPE: &str = "logicalType";
struct SchemaToAvroSchema {
schema: String,
+ /// Names of the `fixed` types defined so far. Avro allows one definition
per
+ /// name, and the visitor reaches fields in the order they're serialized.
+ defined_names: HashSet<Name>,
+}
+
+impl SchemaToAvroSchema {
+ /// Returns a reference to `schema` by name if a type with its name was
+ /// already defined, and `schema` itself otherwise.
+ fn define_once(&mut self, schema: AvroSchema) -> AvroSchema {
+ let name = match &schema {
+ AvroSchema::Fixed(FixedSchema { name, .. })
+ | AvroSchema::Decimal(DecimalSchema {
+ inner: InnerDecimalSchema::Fixed(FixedSchema { name, .. }),
+ ..
+ }) => name.clone(),
+ _ => return schema,
+ };
+ if self.defined_names.insert(name.clone()) {
+ schema
+ } else {
+ AvroSchema::Ref { name }
Review Comment:
> `define_once` now emits `AvroSchema::Ref` for a repeated fixed/decimal,
but `AvroSchemaToSchema` has no `Ref` arm
`avro_schema_to_schema` has no callers outside tests. It's marked
`#[allow(unused)]` with a TODO to fix it once something uses it
([`schema.rs#L598-L602`](https://github.com/apache/iceberg-rust/blob/ecba0b537a1f78ec8125c1a26f6dfab3f95b509e/crates/iceberg/src/avro/schema.rs#L598-L602)).
On `main` it already returns an error for any parsed schema that refers to a
named type by name, because a reference falls through to the error arm of
`primitive`
([`schema.rs#L538-L563`](https://github.com/apache/iceberg-rust/blob/ecba0b537a1f78ec8125c1a26f6dfab3f95b509e/crates/iceberg/src/avro/schema.rs#L538-L563)).
What do you think about adding `Ref` support when something starts calling the
converter?
> Correctness here also leans on the visitor reaching names in serialization
order, which no test pins.
[`test_schema_to_avro_schema_defines_named_types_before_references`](https://github.com/apache/iceberg-rust/blob/91486882b29deb0d97560543e0764aa00a2988fd/crates/iceberg/src/avro/schema.rs#L991-L1029)
covers this. It converts a schema with `decimal(10, 2)` at the top level, in a
list element, and in a map value with `int` keys. It then parses the serialized
Avro schema and checks that `decimal_10_2` is defined once. apache-avro 0.22
fails to parse a reference that comes before its definition ("Unknown primitive
type"), so the parse fails if the visitor order ever differs from the
serialization order.
--
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]
