Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework logical type representation #23

Merged
merged 16 commits into from
Oct 20, 2024
Merged

Conversation

Ten0
Copy link
Owner

@Ten0 Ten0 commented Jul 6, 2024

Resolves #22

What it does:

  • Fixes logical type representation for fixed with logical type ({"type":{"name":"duration","type":"fixed","size":12},"logicalType":"duration"} -> {"type":"fixed", "name":"duration","size":12,"logicalType":"duration"})
    This is what I had originally written but interop tests with apache-avro bamboozled me into thinking this is how it was supposed to be. It wasn't, and they later fixed it to make it homogeneous with Java.
  • Removes support for overly nested types in schema deserialization.
    This means specifically that the old representation above won't parse. (And similar objects written by versions of apache-avro before their fix won't parse.)
    This is not supported by the reference Java implementation, and it's unspecified and unclear how references would resolve for named overly-nested types.
  • Reworks logical type representation: SchemaNode goes from:
    pub enum SchemaNode {
        /// An Avro type that's not annotated with a logical type
        RegularType(RegularType),
        /// An Avro type that is annotated with a logical type
        LogicalType {
            /// The key of the [`RegularType`] (in the [`SchemaMut`]) that is
            /// annotated with this logical type
            inner: SchemaKey,
            /// The LogicalType this node is annotated with
            logical_type: LogicalType,
        },
    }
    to
    pub struct SchemaNode {
        /// The underlying regular type of this node
        pub type_: RegularType,
        /// Logical type that the avro type is annotated with, if any
        pub logical_type: Option<LogicalType>,
    }
    which is simpler to use, and used to be the representation before I got bamboozled into thinking it was not the appropriate representation by interop tests with the broken apache-avro.

@Ten0
Copy link
Owner Author

Ten0 commented Oct 20, 2024

I did not have second thoughts about this so I'm going to pull the trigger and release this as 2.0.

@Ten0 Ten0 merged commit d2456a0 into master Oct 20, 2024
4 checks passed
@Ten0 Ten0 deleted the fix_logical_types_schema_repr branch October 20, 2024 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decimal logical type schema json representation is inconsistent with Java impl
1 participant