martin-g commented on code in PR #339:
URL: https://github.com/apache/avro-rs/pull/339#discussion_r2553945822
##########
avro/src/types.rs:
##########
@@ -478,6 +480,15 @@ impl Value {
None
}
}
+ (&Value::Fixed(n, _), &Schema::Uuid(UuidSchema::Fixed(_))) => {
+ if n != 16 {
+ Some(format!(
+ "The value's size ('{n}') must be exactly 12 to be a
Duration"
Review Comment:
```suggestion
"The value's size ('{n}') must be exactly 16 to be a
Uuid"
```
##########
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: w_name,
+ size: w_size,
+ ..
+ })) => {
+ if let Schema::Uuid(UuidSchema::Fixed(FixedSchema {
+ name: r_name,
+ size: r_size,
+ ..
+ })) = readers_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: r_name,
Review Comment:
```suggestion
name: w_name,
```
##########
avro/src/decode.rs:
##########
@@ -101,39 +102,66 @@ pub(crate) fn decode_internal<R: Read, S: Borrow<Schema>>(
}
}
}
- Schema::Decimal(DecimalSchema { ref inner, .. }) => match &**inner {
- Schema::Fixed { .. } => {
- match decode_internal(inner, names, enclosing_namespace,
reader)? {
+ Schema::Decimal(DecimalSchema { inner, .. }) => match inner {
+ InnerDecimalSchema::Fixed(fixed) => {
+ match decode_internal(
+ &Schema::Fixed(fixed.copy_only_size()),
+ names,
+ enclosing_namespace,
+ reader,
+ )? {
Value::Fixed(_, bytes) =>
Ok(Value::Decimal(Decimal::from(bytes))),
value => Err(Details::FixedValue(value).into()),
}
}
- Schema::Bytes => match decode_internal(inner, names,
enclosing_namespace, reader)? {
- Value::Bytes(bytes) =>
Ok(Value::Decimal(Decimal::from(bytes))),
- value => Err(Details::BytesValue(value).into()),
- },
- schema => Err(Details::ResolveDecimalSchema(schema.into()).into()),
+ InnerDecimalSchema::Bytes => {
+ match decode_internal(&Schema::Bytes, names,
enclosing_namespace, reader)? {
+ Value::Bytes(bytes) =>
Ok(Value::Decimal(Decimal::from(bytes))),
+ value => Err(Details::BytesValue(value).into()),
+ }
+ }
},
Schema::BigDecimal => {
match decode_internal(&Schema::Bytes, names, enclosing_namespace,
reader)? {
Value::Bytes(bytes) =>
deserialize_big_decimal(&bytes).map(Value::BigDecimal),
value => Err(Details::BytesValue(value).into()),
}
}
- Schema::Uuid => {
+ Schema::Uuid(UuidSchema::String) => {
+ let Value::String(string) =
+ decode_internal(&Schema::String, names, enclosing_namespace,
reader)?
+ else {
+ // Calling decode_internal with Schema::String can only return
a Value::String or an error
Review Comment:
Actually it may return `Value::Null` too. See
https://github.com/apache/avro-rs/blob/fdb0f5d6d6b4d137b371663885b7bf797991a16b/avro/src/decode.rs#L211
##########
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: w_name,
Review Comment:
This is the readers schema ^^
Below at line 454 you use again the reader schema
```suggestion
name: r_name,
```
##########
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: w_name,
+ size: w_size,
+ ..
+ })) => {
+ if let Schema::Uuid(UuidSchema::Fixed(FixedSchema {
+ name: r_name,
+ size: r_size,
Review Comment:
```suggestion
size: w_size,
```
##########
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: w_name,
+ size: w_size,
+ ..
+ })) => {
+ if let Schema::Uuid(UuidSchema::Fixed(FixedSchema {
+ name: r_name,
+ size: r_size,
+ ..
+ })) = readers_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: r_name,
+ size: r_size,
Review Comment:
```suggestion
size: w_size,
```
##########
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: w_name,
+ size: w_size,
+ ..
+ })) => {
+ if let Schema::Uuid(UuidSchema::Fixed(FixedSchema {
+ name: r_name,
+ size: r_size,
+ ..
+ })) = readers_schema
Review Comment:
```suggestion
})) = writers_schema
```
##########
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: w_name,
+ size: w_size,
Review Comment:
```suggestion
size: r_size,
```
##########
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: w_name,
+ size: w_size,
+ ..
+ })) => {
+ if let Schema::Uuid(UuidSchema::Fixed(FixedSchema {
+ name: r_name,
Review Comment:
```suggestion
name: w_name,
```
##########
avro/src/schema_compatibility.rs:
##########
@@ -499,8 +535,15 @@ impl SchemaCompatibility {
SchemaKind::String => {
check_reader_type_multi(r_type, vec![SchemaKind::Bytes,
SchemaKind::Uuid], w_type)
}
- SchemaKind::Bytes => check_reader_type(r_type, SchemaKind::String,
w_type),
- SchemaKind::Uuid => check_reader_type(r_type, SchemaKind::String,
w_type),
+ SchemaKind::Bytes => {
+ check_reader_type_multi(r_type, vec![SchemaKind::String,
SchemaKind::Uuid], w_type)
+ }
+ SchemaKind::Uuid => check_reader_type_multi(
+ r_type,
+ vec![SchemaKind::String, SchemaKind::Bytes, SchemaKind::Fixed],
+ w_type,
+ ),
+ SchemaKind::Fixed => check_reader_type(r_type, SchemaKind::Uuid,
w_type),
Review Comment:
This most probably also needs to check the `size`
##########
avro/src/schema_compatibility.rs:
##########
@@ -499,8 +535,15 @@ impl SchemaCompatibility {
SchemaKind::String => {
check_reader_type_multi(r_type, vec![SchemaKind::Bytes,
SchemaKind::Uuid], w_type)
}
- SchemaKind::Bytes => check_reader_type(r_type, SchemaKind::String,
w_type),
- SchemaKind::Uuid => check_reader_type(r_type, SchemaKind::String,
w_type),
+ SchemaKind::Bytes => {
+ check_reader_type_multi(r_type, vec![SchemaKind::String,
SchemaKind::Uuid], w_type)
+ }
+ SchemaKind::Uuid => check_reader_type_multi(
+ r_type,
+ vec![SchemaKind::String, SchemaKind::Bytes, SchemaKind::Fixed],
+ w_type,
+ ),
+ SchemaKind::Fixed => check_reader_type(r_type, SchemaKind::Uuid,
w_type),
Review Comment:
```suggestion
SchemaKind::Fixed => check_reader_type_multi(r_type,
vec![SchemaKind::Duration, SchemaKind::Uuid], w_type),
```
?
##########
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: w_name,
+ size: w_size,
+ ..
+ })) => {
+ if let Schema::Uuid(UuidSchema::Fixed(FixedSchema {
+ name: r_name,
+ size: r_size,
+ ..
+ })) = readers_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: r_name,
+ size: r_size,
+ ..
+ }) = readers_schema
Review Comment:
```suggestion
}) = writers_schema
```
##########
avro/src/decode.rs:
##########
@@ -101,39 +102,66 @@ pub(crate) fn decode_internal<R: Read, S: Borrow<Schema>>(
}
}
}
- Schema::Decimal(DecimalSchema { ref inner, .. }) => match &**inner {
- Schema::Fixed { .. } => {
- match decode_internal(inner, names, enclosing_namespace,
reader)? {
+ Schema::Decimal(DecimalSchema { inner, .. }) => match inner {
+ InnerDecimalSchema::Fixed(fixed) => {
+ match decode_internal(
+ &Schema::Fixed(fixed.copy_only_size()),
Review Comment:
Why this copy is needed ?
##########
avro/src/decode.rs:
##########
@@ -101,39 +102,66 @@ pub(crate) fn decode_internal<R: Read, S: Borrow<Schema>>(
}
}
}
- Schema::Decimal(DecimalSchema { ref inner, .. }) => match &**inner {
- Schema::Fixed { .. } => {
- match decode_internal(inner, names, enclosing_namespace,
reader)? {
+ Schema::Decimal(DecimalSchema { inner, .. }) => match inner {
+ InnerDecimalSchema::Fixed(fixed) => {
+ match decode_internal(
+ &Schema::Fixed(fixed.copy_only_size()),
+ names,
+ enclosing_namespace,
+ reader,
+ )? {
Value::Fixed(_, bytes) =>
Ok(Value::Decimal(Decimal::from(bytes))),
value => Err(Details::FixedValue(value).into()),
}
}
- Schema::Bytes => match decode_internal(inner, names,
enclosing_namespace, reader)? {
- Value::Bytes(bytes) =>
Ok(Value::Decimal(Decimal::from(bytes))),
- value => Err(Details::BytesValue(value).into()),
- },
- schema => Err(Details::ResolveDecimalSchema(schema.into()).into()),
+ InnerDecimalSchema::Bytes => {
+ match decode_internal(&Schema::Bytes, names,
enclosing_namespace, reader)? {
+ Value::Bytes(bytes) =>
Ok(Value::Decimal(Decimal::from(bytes))),
+ value => Err(Details::BytesValue(value).into()),
+ }
+ }
},
Schema::BigDecimal => {
match decode_internal(&Schema::Bytes, names, enclosing_namespace,
reader)? {
Value::Bytes(bytes) =>
deserialize_big_decimal(&bytes).map(Value::BigDecimal),
value => Err(Details::BytesValue(value).into()),
}
}
- Schema::Uuid => {
+ Schema::Uuid(UuidSchema::String) => {
+ let Value::String(string) =
+ decode_internal(&Schema::String, names, enclosing_namespace,
reader)?
+ else {
+ // Calling decode_internal with Schema::String can only return
a Value::String or an error
+ unreachable!()
+ };
+ let uuid =
Uuid::parse_str(&string).map_err(Details::ConvertStrToUuid)?;
+ Ok(Value::Uuid(uuid))
+ }
+ Schema::Uuid(UuidSchema::Bytes) => {
let Value::Bytes(bytes) =
decode_internal(&Schema::Bytes, names, enclosing_namespace,
reader)?
else {
// Calling decode_internal with Schema::Bytes can only return
a Value::Bytes or an error
- unreachable!();
+ unreachable!()
Review Comment:
Maybe move the comment to be a parameter of the `unreachable!("... here
...")` ?!
There are few more such occurrences below.
--
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]
