Copilot commented on code in PR #339:
URL: https://github.com/apache/avro-rs/pull/339#discussion_r2639707773
##########
avro/src/schema.rs:
##########
@@ -894,7 +923,40 @@ pub struct DecimalSchema {
/// The number of digits to the right of the decimal point
pub scale: DecimalMetadata,
/// The inner schema of the decimal (fixed or bytes)
- pub inner: Box<Schema>,
+ pub inner: InnerDecimalSchema,
+}
+
+/// The inner schema of the Decimal type.
+#[derive(Debug, Clone)]
+pub enum InnerDecimalSchema {
+ Bytes,
+ Fixed(FixedSchema),
+}
+
+impl TryFrom<Schema> for InnerDecimalSchema {
+ type Error = Error;
+
+ fn try_from(value: Schema) -> Result<Self, Self::Error> {
+ match value {
+ Schema::Bytes => Ok(InnerDecimalSchema::Bytes),
+ Schema::Fixed(fixed) => Ok(InnerDecimalSchema::Fixed(fixed)),
+ _ => Err(Details::ResolveDecimalSchema(value.into()).into()),
+ }
+ }
+}
+
+/// The inner schema of the Uuid type.
+#[derive(Debug, Clone)]
+pub enum UuidSchema {
+ /// [`Schema::Bytes`] with size of 16.
+ ///
+ /// This is according to specification, but was what happened in `0.21.0`
and earlier when
+ /// a schema with logical type `uuid` and inner type `fixed` was used.
Review Comment:
The documentation comment states "This is according to specification" but
this seems incorrect. The Bytes variant is explicitly noted as NOT according to
specification - it's a legacy compatibility feature from version 0.21.0 and
earlier. The comment should clarify that Bytes is NOT spec-compliant.
```suggestion
/// This is NOT according to the Avro specification; it is a legacy
compatibility feature,
/// preserving the behavior of `0.21.0` and earlier when a schema with
logical type `uuid`
/// and inner type `fixed` was used.
```
##########
avro/src/schema_compatibility.rs:
##########
@@ -1070,11 +1180,10 @@ mod tests {
(nested_optional_record(), nested_record()),
];
- assert!(
- compatible_schemas
- .iter()
- .all(|(reader, writer)| SchemaCompatibility::can_read(writer,
reader).is_ok())
- );
+ for (reader, writer) in compatible_schemas {
+ println!("{reader:?}, {writer:?}");
Review Comment:
This debug println statement appears to be leftover debug code that should
be removed before merging. It's adding unnecessary output during test execution.
```suggestion
```
##########
avro/src/types.rs:
##########
@@ -478,6 +480,15 @@ impl Value {
None
}
}
+ (&Value::Fixed(n, _), Schema::Uuid(UuidSchema::Fixed(size, ..)))
=> {
+ if n != 16 || size.size != 16 {
Review Comment:
The pattern matching uses `Schema::Uuid(UuidSchema::Fixed(size, ..))` but
`UuidSchema::Fixed` only has one field (a `FixedSchema`), not a tuple. This
should be `Schema::Uuid(UuidSchema::Fixed(FixedSchema { size, .. }))` to
properly destructure the `FixedSchema` struct.
```suggestion
(&Value::Fixed(n, _), Schema::Uuid(UuidSchema::Fixed(FixedSchema
{ size, .. }))) => {
if n != 16 || size != 16 {
```
--
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]
