martin-g commented on code in PR #339:
URL: https://github.com/apache/avro-rs/pull/339#discussion_r2597971648
##########
avro/src/types.rs:
##########
@@ -1870,7 +1886,34 @@ Field with name '"b"' is not a member of the map items"#,
#[test]
fn resolve_uuid() -> TestResult {
let value =
Value::Uuid(Uuid::parse_str("1481531d-ccc9-46d9-a56f-5b67459c0537")?);
- assert!(value.clone().resolve(&Schema::Uuid).is_ok());
+ assert!(
+ value
+ .clone()
+ .resolve(&Schema::Uuid(UuidSchema::String))
+ .is_ok()
+ );
+ assert!(
+ value
+ .clone()
+ .resolve(&Schema::Uuid(UuidSchema::Bytes))
+ .is_ok()
+ );
+ assert!(
+ value
+ .clone()
+ .resolve(&Schema::Uuid(UuidSchema::Fixed(FixedSchema {
+ name: Name {
Review Comment:
```suggestion
name: Name::new("some dummy name")?
```
instead of using an invalid Name
##########
avro/tests/serde_human_readable_true.rs:
##########
@@ -18,8 +18,7 @@ fn avro_rs_53_uuid_with_fixed_true() -> TestResult {
"fields" : [ {
"name" : "id",
"type" : {
- "type" : "fixed",
- "size" : 16,
+ "type" : "string",
Review Comment:
We need to rename the function. Currently it still says
`uuid_with_fixed_true`
##########
avro/src/schema_compatibility.rs:
##########
@@ -426,13 +427,48 @@ impl SchemaCompatibility {
}
}
}
- SchemaKind::Uuid => {
- return check_writer_type(
- writers_schema,
- readers_schema,
- vec![r_type, SchemaKind::String],
- );
- }
+ SchemaKind::Uuid => match readers_schema {
+ Schema::Uuid(UuidSchema::Bytes) => {
+ return check_writer_type(
+ writers_schema,
+ readers_schema,
+ vec![r_type, SchemaKind::Bytes],
+ );
+ }
+ Schema::Uuid(UuidSchema::String) => {
+ return check_writer_type(
+ writers_schema,
+ readers_schema,
+ vec![r_type, SchemaKind::String],
+ );
+ }
+ Schema::Uuid(UuidSchema::Fixed(FixedSchema {
+ name: r_name,
+ size: r_size,
+ ..
+ })) => {
+ if let Schema::Uuid(UuidSchema::Fixed(FixedSchema {
+ name: w_name,
+ size: w_size,
+ ..
+ })) = writers_schema
+ {
+ return (w_name.name == r_name.name && w_size ==
r_size)
+ .then_some(())
+ .ok_or(CompatibilityError::FixedMismatch);
+ } else if let Schema::Fixed(FixedSchema {
+ name: w_name,
+ size: w_size,
+ ..
+ }) = writers_schema
+ {
+ return (w_name.name == r_name.name && w_size ==
r_size)
+ .then_some(())
+ .ok_or(CompatibilityError::FixedMismatch);
+ }
Review Comment:
We need to return an Err in an `else` clause here.
##########
avro/src/schema_equality.rs:
##########
@@ -544,4 +556,60 @@ mod tests {
assert_eq!(specification_eq_res, struct_field_eq_res);
Ok(())
}
+
+ #[test]
+ fn test_uuid_compare_uuid() -> TestResult {
+ let string = Schema::Uuid(UuidSchema::String);
+ let bytes = Schema::Uuid(UuidSchema::Bytes);
+ let mut fixed_schema = FixedSchema {
+ name: Name {
+ name: String::new(),
+ namespace: None,
+ },
+ aliases: None,
+ doc: None,
+ size: 16,
+ default: None,
+ attributes: Default::default(),
+ };
+ let fixed = Schema::Fixed(fixed_schema.clone());
Review Comment:
```suggestion
let fixed = Schema::Uuid(UuidSchema::Fixed(fixed_schema.clone()));
```
##########
avro/src/schema_equality.rs:
##########
@@ -544,4 +556,60 @@ mod tests {
assert_eq!(specification_eq_res, struct_field_eq_res);
Ok(())
}
+
+ #[test]
+ fn test_uuid_compare_uuid() -> TestResult {
+ let string = Schema::Uuid(UuidSchema::String);
+ let bytes = Schema::Uuid(UuidSchema::Bytes);
+ let mut fixed_schema = FixedSchema {
+ name: Name {
Review Comment:
```suggestion
name: Name::new("some dummy name")
```
##########
avro/src/schema.rs:
##########
@@ -2539,7 +2618,20 @@ pub mod derive {
impl_schema!(f32, Schema::Float);
impl_schema!(f64, Schema::Double);
impl_schema!(String, Schema::String);
- impl_schema!(uuid::Uuid, Schema::Uuid);
+ impl_schema!(
+ uuid::Uuid,
+ Schema::Uuid(UuidSchema::Fixed(FixedSchema {
+ name: Name {
+ name: String::new(),
Review Comment:
Empty name is against the specification.
Maybe we should use `Schema::Uuid(UuidSchema::String)` or
`Schema::Uuid(UuidSchema::Bytes)` instead ?!
##########
avro/src/schema_equality.rs:
##########
@@ -544,4 +556,60 @@ mod tests {
assert_eq!(specification_eq_res, struct_field_eq_res);
Ok(())
}
+
+ #[test]
+ fn test_uuid_compare_uuid() -> TestResult {
+ let string = Schema::Uuid(UuidSchema::String);
+ let bytes = Schema::Uuid(UuidSchema::Bytes);
+ let mut fixed_schema = FixedSchema {
+ name: Name {
+ name: String::new(),
+ namespace: None,
+ },
+ aliases: None,
+ doc: None,
+ size: 16,
+ default: None,
+ attributes: Default::default(),
+ };
+ let fixed = Schema::Fixed(fixed_schema.clone());
+ fixed_schema
+ .attributes
+ .insert("Something".to_string(), Value::Null);
+ let fixed_different = Schema::Fixed(fixed_schema);
Review Comment:
```suggestion
let fixed_different =
Schema::Uuid(UuidSchema::Fixed(fixed_schema.clone()));
```
##########
avro/src/schema.rs:
##########
@@ -894,7 +911,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)]
Review Comment:
```suggestion
#[derive(Debug, Clone)]
#[non_exhaustive]
```
Would that help to avoid the `unreachable!()` at
https://github.com/apache/avro-rs/pull/339/files#diff-7bb1045091377e3af9ff4a97817d8362470be45ba74a8327211241b30a0f8b89R470
?
It would be better to have a compilation error when a new variant is added
than crashing the app with `unreachable!()`
--
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]
