martin-g commented on code in PR #382:
URL: https://github.com/apache/avro-rs/pull/382#discussion_r2652371215
##########
avro/src/serde/de.rs:
##########
@@ -354,9 +354,13 @@ impl<'de> de::Deserializer<'de> for &Deserializer<'de> {
Value::BigDecimal(ref big_decimal) => {
visitor.visit_str(big_decimal.to_plain_string().as_str())
}
- _ => Err(de::Error::custom(format!(
Review Comment:
This is no more needed because the check is exhaustive, right ?
##########
avro/src/schema.rs:
##########
Review Comment:
Please add support for Schema::Duration here too
##########
avro/src/schema_equality.rs:
##########
@@ -268,6 +267,29 @@ mod tests {
test_primitives!(LocalTimestampMillis);
test_primitives!(LocalTimestampNanos);
+ #[test]
+ fn avro_rs_382_compare_schemata_duration() {
+ let schema_one = Schema::Duration(FixedSchema {
+ name: Name::from("name1"),
+ size: 12,
+ aliases: None,
+ doc: None,
+ default: None,
+ attributes: BTreeMap::new(),
+ });
+ let schema_two = Schema::Duration(FixedSchema {
+ name: Name::from("name1"),
+ size: 12,
+ aliases: None,
+ doc: None,
+ default: None,
+ attributes: BTreeMap::new(),
+ });
+ let specification_eq_res = SPECIFICATION_EQ.compare(&schema_one,
&schema_two);
+ let struct_field_eq_res = STRUCT_FIELD_EQ.compare(&schema_one,
&schema_two);
+ assert_eq!(specification_eq_res, struct_field_eq_res)
+ }
Review Comment:
Let's add two more (negative) tests:
1) schemas with different names
2) schemas with different sizes
##########
avro/src/schema.rs:
##########
@@ -134,7 +134,7 @@ pub enum Schema {
/// An instant in local time represented as the number of nanoseconds
after the UNIX epoch.
LocalTimestampNanos,
/// An amount of time defined by a number of months, days and milliseconds.
- Duration,
+ Duration(FixedSchema),
Review Comment:
I am not sure about this.
To prevent API breaks in the future it would be better to use
`Duration(DurationSchema)` where:
```rust
enum DurationSchema {
Fixed(FixedSchema)
}
```
This way if some day Duration is represented by another (e.g. more compact)
way it will be easier to add a second variant to DurationSchema.
Similar to UuidSchema.
##########
avro/src/types.rs:
##########
Review Comment:
Old issue: `GetDecimalFixedBytes` is not the correct error type here.
```suggestion
return Err(Details::ResolveDuration(Value::Fixed(size,
bytes.clone())).into());
```
##########
avro/src/schema.rs:
##########
Review Comment:
And here
##########
avro/src/schema.rs:
##########
Review Comment:
Please add an arm for Schema::Duration too.
Maybe also a new test for schema equality. Currently two duration schemas
with different attributes would match or not depending on `include_attributes`
##########
avro/src/duration.rs:
##########
@@ -1,3 +1,5 @@
+use serde::{Deserialize, Serialize, de};
Review Comment:
Please move this import below the ASF licence header
##########
avro/src/schema.rs:
##########
Review Comment:
Please add Schema::Duration support here
--
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]
