martin-g commented on code in PR #339:
URL: https://github.com/apache/avro-rs/pull/339#discussion_r2630385962
##########
avro/src/serde/ser_schema.rs:
##########
@@ -1186,7 +1190,7 @@ impl<'s, W: Write> SchemaAwareWriteSerializer<'s, W> {
match variant_schema {
Schema::String
| Schema::Bytes
- | Schema::Uuid
+ | Schema::Uuid(UuidSchema::Bytes)
Review Comment:
What about `Schema::Uuid(UuidSchema::Fixed(..))` ?
##########
avro/src/writer.rs:
##########
@@ -990,41 +991,41 @@ mod tests {
#[test]
fn decimal_fixed() -> TestResult {
let size = 30;
- let inner = Schema::Fixed(FixedSchema {
+ let fixed = FixedSchema {
name: Name::new("decimal")?,
aliases: None,
doc: None,
size,
default: None,
attributes: Default::default(),
- });
+ };
+ let inner = InnerDecimalSchema::Fixed(fixed.clone());
let value = vec![0u8; size];
logical_type_test(
r#"{"type": {"type": "fixed", "size": 30, "name": "decimal"},
"logicalType": "decimal", "precision": 20, "scale": 5}"#,
&Schema::Decimal(DecimalSchema {
precision: 20,
scale: 5,
- inner: Box::new(inner.clone()),
+ inner: inner.clone(),
Review Comment:
nit: no need to clone, I think
##########
avro/src/schema.rs:
##########
@@ -1547,7 +1617,9 @@ impl Parser {
Ok((precision, scale)) =>
Ok(Schema::Decimal(DecimalSchema {
precision,
scale,
- inner: Box::new(inner),
+ inner: inner.try_into().unwrap_or_else(|_|
{
+ unreachable!("inner is Fixed or Bytes")
Review Comment:
Do we really need to unwrap here ?
`inner.try_into()?` should be enough, no ? It will return an Err -
`Err(Details::ResolveDecimalSchema(value.into()).into()),`
##########
avro/src/schema_equality.rs:
##########
@@ -143,9 +142,23 @@ impl SchemataEq for StructFieldEq {
Schema::Decimal(DecimalSchema { precision: precision_one,
scale: scale_one, inner: inner_one }),
Schema::Decimal(DecimalSchema { precision: precision_two,
scale: scale_two, inner: inner_two })
) => {
- precision_one == precision_two && scale_one == scale_two &&
self.compare(inner_one, inner_two)
+ precision_one == precision_two && scale_one == scale_two &&
match (inner_one, inner_two) {
+ (InnerDecimalSchema::Bytes, InnerDecimalSchema::Bytes) =>
true,
+ (InnerDecimalSchema::Fixed(FixedSchema { size: size_one,
.. }), InnerDecimalSchema::Fixed(FixedSchema { size: size_two, ..})) => {
Review Comment:
Do we need to compare the names here too ?!
##########
avro/src/schema.rs:
##########
Review Comment:
I think we should add an arm for `Schema::Uuid(UuidSchema::Fixed(FixedSchema
{ name, ..}))` here.
And add a test that resolves a Schema::Ref for it
##########
avro/src/types.rs:
##########
@@ -478,6 +480,15 @@ impl Value {
None
}
}
+ (&Value::Fixed(n, _), &Schema::Uuid(UuidSchema::Fixed(_))) => {
Review Comment:
```suggestion
(&Value::Fixed(n, _), &Schema::Uuid(UuidSchema::Fixed(size,
..))) => {
```
Should we check that `size` is also `16` here ?
##########
avro/src/schema.rs:
##########
@@ -111,7 +111,7 @@ pub enum Schema {
/// The underlying type is serialized and deserialized as `Schema::Bytes`
BigDecimal,
/// A universally unique identifier, annotating a string.
Review Comment:
`annotating a string` need to be updated that it supports also Bytes and
Fixed now
##########
avro/src/schema.rs:
##########
Review Comment:
I think we should add an arm for `Schema::Uuid(UuidSchema::Fixed(FixedSchema
{ name, ..}))` here.
And add a test that resolves a Schema::Ref for it
##########
avro/src/schema.rs:
##########
@@ -6713,14 +6792,38 @@ mod tests {
"logicalType": "uuid"
});
let parse_result = Schema::parse(&schema)?;
- assert_eq!(parse_result, Schema::Uuid);
+ assert_eq!(
+ parse_result,
+ Schema::Uuid(UuidSchema::Fixed(FixedSchema {
+ name: Name::new("FixedUUID")?,
+ aliases: None,
+ doc: None,
+ size: 16,
+ default: None,
+ attributes: Default::default(),
+ }))
+ );
assert_not_logged(
r#"Ignoring uuid logical type for a Fixed schema because its size
(6) is not 16! Schema: Fixed(FixedSchema { name: Name { name: "FixedUUID",
namespace: None }, aliases: None, doc: None, size: 6, attributes:
{"logicalType": String("uuid")} })"#,
);
Ok(())
}
+ #[test]
+ fn uuid_schema_bytes() -> TestResult {
+ let schema = json!(
+ {
+ "type": "bytes",
+ "name": "BytesUUID",
+ "logicalType": "uuid"
+ });
+ let parse_result = Schema::parse(&schema)?;
+ assert_eq!(parse_result, Schema::Uuid(UuidSchema::Bytes));
+
+ Ok(())
+ }
+
Review Comment:
Do we have a similar test already:
```rust
#[test]
fn test_uuid_fixed_invalid_size() {
let schema = Schema::parse_str(r#"{
"type": "fixed",
"size": 8,
"logicalType": "uuid",
"name": "BadUUID"
}"#)?;
}
```
It should fail due to `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]
