Preserve unrecognized logical types and properties #469
Conversation
…move LogicalType field from intermediate structs that don't really use it
jhump
force-pushed
the
jh/preserve-unrecognized
branch
from
84d6ba2
to
7576339
Compare
schema_parse.go
Outdated
| func checkLogicalType(props map[string]any) error { | ||
| val, ok := props["logicalType"] | ||
| if !ok { | ||
| return nil | ||
| } | ||
| if _, isString := val.(string); !isString { | ||
| return fmt.Errorf(`"logicalType" attribute must be a string, got %T`, val) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Out of interest, what necessitates logicalType in this context be a string? At this point it is metadata, and by spec could be any type.
There was a problem hiding this comment.
This is for consistency. The way logicalType is parsed for other fixed and primitive types, it will be an error if it is not a string. Why would it be an error there but not for the other kinds of types? The spec doesn't say anything about the attribute being specific to fixed or primitive types:
A logical type is an Avro primitive or complex type with extra attributes to represent a derived type. The attribute
logicalTypemust always be present for a logical type, and is a string with the name of one of the logical types listed later in this section. Other attributes may be defined for particular logical types.
So if it's not an error here, and we allow treating a non-string value as an extra property, then I should probably also change the way fixed and primitive types are parsed, to similarly allow non-string values which would result in it just being another property.
There was a problem hiding this comment.
Okay, I reworked it so that logical type does not have to be a string; it's just another property. For fixed and primitive types, if it's not a string, it's also tossed into the set of other properties and otherwise ignored.
Does that look better?
…unrecognized logicalType attributes
…o into other props, even for fixed and primitive
This updates the parsing of schema JSON documents and the way properties are possibly discarded (to avoid problematic properties that conflict with proper fields of a type and could result in invalid JSON if included when marshaling to JSON).
The tests now demonstrate that all properties and logical types are preserved as extra properties, even for unrecognized and incorrectly configured logical types, even for null types.
I added these everywhere so that this property would be properly type-checked: if it is present, it should be a string. So now this will surface as an error if it's not a string, even for an "array" or "record" type.
Resolves #435.