Duration inner fixed schema and serialization #382

jdarais · 2025-12-29T21:33:34Z

This PR adds some missing functionality for the Duration type:

Schema::Duration is updated to contain a FixedSchema struct, which contains name, namespace, size, doc, etc. fields that can exist on Duration types. (Name and Namespace fields for Schema::Duration #378)
apache_avro's Duration type is updated to implement Serialize and Deserialize to support inclusion in structs de/serialized from/to avro "the serde way"
Small fix to the Deserializer implementation in serde/de.rs to allow Value::Duration to be deserialized. (Duration is deserialized as bytes.)

This PR also deprecates SchemaKind::is_named in favor of a new Schema::is_named function, which can correctly determine whether a schema is for a named type, even when it is for a logical type that could have either a named or non-named underlying type, (such as "decimal" or "uuid".)

One note on this change: I noticed that the derive macro for AvroSchema was using the Duration schema for core::time::Duration. It is no longer trivial to create a schema for a Duration type, since the schema would need to be given a name. It looks like other named types are excluded from these derive macros, so I just deleted the impl_schema!(core::time::Duration, Schema::Duration) line, but I'm not sure if there's something better that can be done here.

Kriskras99

First glance looks good, I'll have a more in depth look tomorrow

Kriskras99 · 2025-12-29T21:51:49Z

avro/src/decode.rs

        Schema::LocalTimestampMicros => zag_i64(reader).map(Value::LocalTimestampMicros),
        Schema::LocalTimestampNanos => zag_i64(reader).map(Value::LocalTimestampNanos),
-        Schema::Duration => {
+        Schema::Duration(_) => {


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

Kriskras99 · 2025-12-29T21:52:41Z

avro/src/duration.rs

+    }
+}
+
+struct DurationVisitor;


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

Kriskras99 · 2025-12-29T21:53:44Z

avro/src/duration.rs

+    use apache_avro_test_helper::TestResult;
+
+    #[test]
+    fn test_duration_from_value() -> TestResult {


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

Kriskras99 · 2025-12-29T21:57:04Z

avro/src/schema_equality.rs

    test_primitives!(LocalTimestampNanos);

+    #[test]
+    fn test_avro_3939_compare_schemata_duration() {


This should be avro_rs_382

…bytes for Duration

jdarais · 2025-12-30T05:31:34Z

@Kriskras99 thanks for the feedback, I made the suggested changes

martin-g · 2025-12-30T07:07:00Z

avro/src/serde/de.rs

                Value::BigDecimal(ref big_decimal) => {
                    visitor.visit_str(big_decimal.to_plain_string().as_str())
                }
-                _ => Err(de::Error::custom(format!(


This is no more needed because the check is exhaustive, right ?

martin-g · 2025-12-30T07:17:17Z

avro/src/schema.rs

    LocalTimestampNanos,
    /// An amount of time defined by a number of months, days and milliseconds.
-    Duration,
+    Duration(FixedSchema),


I am not sure about this.
To prevent API breaks in the future it would be better to use Duration(DurationSchema) where:

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.

I suspect that switching from Duration(FixedSchema) to Duration(DurationSchema) down the road may not be a whole lot more disruptive than starting with Duration(DurationSchema) and adding a new enum value down the road. In the latter case, library consumers would still need to update match statements to accommodate the new enum variant, and nothing would have prevented consumers from writing code that assumes that duration is a fixed type.

Admittedly, having to migrate consumer code from using a FixedSchema to using an enum would be slightly more disruptive, but I think it would be worth it to not have to deal with the extra enum layer for now. Let me know what you think.

martin-g · 2025-12-30T07:37:43Z

avro/src/schema.rs

                inner: InnerDecimalSchema::Fixed(FixedSchema { attributes, .. }),
                ..
            })
            | Schema::Uuid(UuidSchema::Fixed(FixedSchema { attributes, .. })) => Some(attributes),


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

martin-g · 2025-12-30T07:39:11Z

avro/src/schema.rs

                inner: InnerDecimalSchema::Fixed(FixedSchema { aliases, .. }),
                ..
            })
            | Schema::Uuid(UuidSchema::Fixed(FixedSchema { aliases, .. })) => aliases.as_ref(),


Please add support for Schema::Duration here too

martin-g · 2025-12-30T07:39:32Z

avro/src/schema.rs

                inner: InnerDecimalSchema::Fixed(FixedSchema { doc, .. }),
                ..
            })
            | Schema::Uuid(UuidSchema::Fixed(FixedSchema { doc, .. })) => doc.as_ref(),


martin-g · 2025-12-30T07:41:02Z

avro/src/types.rs

            duration @ Value::Duration { .. } => duration,
            Value::Fixed(size, bytes) => {
                if size != 12 {
                    return Err(Details::GetDecimalFixedBytes(size).into());


Old issue: GetDecimalFixedBytes is not the correct error type here.

Suggested change

return Err(Details::ResolveDuration(Value::Fixed(size, bytes.clone())).into());

martin-g · 2025-12-30T07:44:59Z

avro/src/schema_equality.rs

+        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)
+    }


Let's add two more (negative) tests:

schemas with different names

schemas with different sizes

martin-g · 2025-12-30T07:49:10Z

avro/src/duration.rs

@@ -1,3 +1,5 @@
+use serde::{Deserialize, Serialize, de};


Please move this import below the ASF licence header

martin-g · 2025-12-30T07:51:57Z

avro/src/schema.rs

                Schema::Enum(EnumSchema { name, .. })
                | Schema::Fixed(FixedSchema { name, .. })
                | Schema::Uuid(UuidSchema::Fixed(FixedSchema { name, .. }))
                | Schema::Decimal(DecimalSchema {


Please add Schema::Duration support here

jdarais · 2026-01-04T03:01:00Z

@martin-g thanks for all the feedback! I should have taken a closer look at the Schema enum and schema comparison functions. I made fixes and added some tests to address all but one of your comments, and on the one remaining one, I responded and would be interested in your thoughts.

Jeremiah Darais added 4 commits December 29, 2025 09:56

Add Duration to Deserializer, Add FixedSchema to Schema::Duration

66c04cc

Deprecate SchemaKind::is_named in favor of Schema::is_named

22091f3

formatting

320f1e2

fmt and clippy fixes

cb94a87

Kriskras99 reviewed Dec 29, 2025

View reviewed changes

Jeremiah Darais added 3 commits December 29, 2025 20:11

test names and DurationVisitor cleanup, fix Deserializer deserialize_…

2bfa800

…bytes for Duration

fix duratin schema serialization and test names

31be918

add check for size in duration schema in decode_internal

61df3f7

claude bot mentioned this pull request Dec 30, 2025

382: Duration inner fixed schema and serialization martin-augment/avro-rs#18

Open

martin-g reviewed Dec 30, 2025

View reviewed changes

PR feedback fixes

e33047b


	return Err(Details::ResolveDuration(Value::Fixed(size, bytes.clone())).into());

Duration inner fixed schema and serialization #382

Are you sure you want to change the base?

Duration inner fixed schema and serialization #382

Uh oh!

Conversation

jdarais commented Dec 29, 2025

Uh oh!

Kriskras99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdarais commented Dec 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdarais commented Jan 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants