Add custom logical types #448
Conversation
nrwiersma
left a comment
nrwiersma
left a comment
There was a problem hiding this comment.
Sorry for the delay. This is a rather large change to how schemas function, and in no way can it be turned off for people wanting spec behaviour. I am wondering if there is another way that has less impact on the "normal" behaviour of the package.
| ltyp LogicalType | ||
| } | ||
|
|
||
| var customLogicalSchemas sync.Map // map[customSchemaKey]*CustomLogicalSchema |
There was a problem hiding this comment.
This being global is an issue. I should be possible to parse with and without the custom logical types.
| } | ||
|
|
||
| // Logical returns the logical schema or nil. | ||
| func (s *RecordSchema) Logical() LogicalSchema { |
There was a problem hiding this comment.
This means that every schema now implements LogicalTypeSchema. From a spec PoV this is really confusing.
| } | ||
|
|
||
| if s.logical != nil { | ||
| return fmt.Sprintf("{\"name\":\"%s\", \"type\":\"%s\", \"fields\":[%s]\", %s}", |
There was a problem hiding this comment.
fmt.Sprintf is slow and includes many allocations. Given this function is called often, this is a performance bottleneck.
| Doc string `mapstructure:"doc"` | ||
| Default any `mapstructure:"default"` | ||
| Order Order `mapstructure:"order"` | ||
| LogicalType string `mapstructure:"logicalType"` |
There was a problem hiding this comment.
A field for sure should not have a logical type, as it is not a type.
|
Hi @nrwiersma Thank you for the feedback! I totally get what you are saying, there ought to be a more elegant approach. Do you have another approach in mind? |
|
I am toying with the idea of adding something along the lines of Parser Hooks, which would allow new types to be derived from the Avro Schema types, but extended to any specific need. My hope is that the hooks would allow arb parsing of parameters into a Schema structure without having to actually change the underlying types. The thought is still a work in progress though. |
2 participants
closes #435
Hi @nrwiersma
This PR adds the possibility to register custom logical types for the schema. These logicaltypes are primitive, and does not include logic for handling custom encoding/decoding, but simply adds the the custom logicalType to be referenced in the schemas.
I have added the
WithCustomLogicalTypefunction as a SchemaOption, but not at all schemas use this option - I saw this pattern was already used for other schema options.Let me know what you think :)