Kriskras99 commented on code in PR #382:
URL: https://github.com/apache/avro-rs/pull/382#discussion_r2651839699


##########
avro/src/schema_equality.rs:
##########
@@ -268,6 +267,29 @@ mod tests {
     test_primitives!(LocalTimestampMillis);
     test_primitives!(LocalTimestampNanos);
 
+    #[test]
+    fn test_avro_3939_compare_schemata_duration() {

Review Comment:
   This should be `avro_rs_382`



##########
avro/src/duration.rs:
##########
@@ -134,12 +136,102 @@ impl From<Duration> for [u8; 12] {
     }
 }
 
-impl From<[u8; 12]> for Duration {
-    fn from(bytes: [u8; 12]) -> Self {
+impl From<Duration> for [u8; 12] {
+    fn from(duration: Duration) -> Self {
+        (&duration).into()
+    }
+}
+
+impl From<&[u8; 12]> for Duration {
+    fn from(bytes: &[u8; 12]) -> Self {
         Self {
             months: Months::from([bytes[0], bytes[1], bytes[2], bytes[3]]),
             days: Days::from([bytes[4], bytes[5], bytes[6], bytes[7]]),
             millis: Millis::from([bytes[8], bytes[9], bytes[10], bytes[11]]),
         }
     }
 }
+
+impl From<[u8; 12]> for Duration {
+    fn from(bytes: [u8; 12]) -> Duration {
+        (&bytes).into()
+    }
+}
+
+impl Serialize for Duration {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        let value_bytes: [u8; 12] = self.into();
+        serializer.serialize_bytes(&value_bytes)
+    }
+}
+
+struct DurationVisitor;

Review Comment:
   Inline this struct and the implementation into the `deserialize` function, 
there is no value in it existing outside of `Deserialize`



##########
avro/src/duration.rs:
##########
@@ -134,12 +136,102 @@ impl From<Duration> for [u8; 12] {
     }
 }
 
-impl From<[u8; 12]> for Duration {
-    fn from(bytes: [u8; 12]) -> Self {
+impl From<Duration> for [u8; 12] {
+    fn from(duration: Duration) -> Self {
+        (&duration).into()
+    }
+}
+
+impl From<&[u8; 12]> for Duration {
+    fn from(bytes: &[u8; 12]) -> Self {
         Self {
             months: Months::from([bytes[0], bytes[1], bytes[2], bytes[3]]),
             days: Days::from([bytes[4], bytes[5], bytes[6], bytes[7]]),
             millis: Millis::from([bytes[8], bytes[9], bytes[10], bytes[11]]),
         }
     }
 }
+
+impl From<[u8; 12]> for Duration {
+    fn from(bytes: [u8; 12]) -> Duration {
+        (&bytes).into()
+    }
+}
+
+impl Serialize for Duration {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        let value_bytes: [u8; 12] = self.into();
+        serializer.serialize_bytes(&value_bytes)
+    }
+}
+
+struct DurationVisitor;
+
+impl<'de> de::Visitor<'de> for DurationVisitor {
+    type Value = Duration;
+
+    fn expecting(&self, f: &mut std::fmt::Formatter) -> Result<(), 
std::fmt::Error> {
+        write!(f, "a byte array with size 12")
+    }
+
+    fn visit_bytes<E>(self, v: &[u8]) -> Result<Self::Value, E>
+    where
+        E: de::Error,
+    {
+        if v.len() != 12 {
+            Err(E::custom(format!(
+                "Expected byte array of length 12, but length is {}",
+                v.len()
+            )))
+        } else {
+            let v_slice: [u8; 12] = v[..12].try_into().unwrap();
+            Ok(Duration::from(v_slice))
+        }
+    }
+}
+
+impl<'de> Deserialize<'de> for Duration {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        deserializer.deserialize_bytes(DurationVisitor)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::types::Value;
+    use anyhow::anyhow;
+    use apache_avro_test_helper::TestResult;
+
+    #[test]
+    fn test_duration_from_value() -> TestResult {

Review Comment:
   Change all new tests to start with `avro_rs_382`, so we can easily get back 
to the PR



##########
avro/src/decode.rs:
##########
@@ -179,7 +179,7 @@ pub(crate) fn decode_internal<R: Read, S: Borrow<Schema>>(
         Schema::LocalTimestampMillis => 
zag_i64(reader).map(Value::LocalTimestampMillis),
         Schema::LocalTimestampMicros => 
zag_i64(reader).map(Value::LocalTimestampMicros),
         Schema::LocalTimestampNanos => 
zag_i64(reader).map(Value::LocalTimestampNanos),
-        Schema::Duration => {
+        Schema::Duration(_) => {

Review Comment:
   It would be good to check if the FixedSchema is of size 12 (and error 
otherwise)



-- 
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]

Reply via email to