Logical types for all types: null,bool,float,double,enum,map,array#2
Logical types for all types: null,bool,float,double,enum,map,array#2yoricksijsling wants to merge 1 commit into
Conversation
| @@ -23,32 +23,32 @@ import qualified Data.Avro.Schema.ReadSchema as Read | |||
| -- Schema resolution rules are described by the specification: <https://avro.apache.org/docs/current/spec.html#Schema+Resolution> | |||
| deconflict :: Schema -> Schema -> Either String ReadSchema | |||
| deconflict writerSchema readerSchema | writerSchema == readerSchema = pure (Read.fromSchema readerSchema) | |||
There was a problem hiding this comment.
I am not sure this implementation is correct. From the specification it remains quite vague whether logical types should be considered equal to their primitive types, and whether two differently-named logical types (but with the same or compatible underlying types) are compatible or not.
We do see this mention for decimal however:
For the purposes of schema resolution, two schemas that are decimal logical types match if their scales and precisions match.
So I think but am not sure, that generally speaking the logical type should be considered during schema resolution, and that mixing logical type names is an error.
| import Avro.Data.Logical | ||
| import Data.Avro (decodeValue, decodeValueWithSchema, encodeValue, encodeValueWithSchema) | ||
| import Data.Avro.Schema.ReadSchema (fromSchema) | ||
| import qualified Data.Aeson as A |
There was a problem hiding this comment.
Style nit: In 8/10 places Data.Aeson is imported as Aeson. There is one other place where it is imported as A and one where it is imported as J (?!)
Let's keep it as Aeson.
| } | ||
| deriving (Ord, Show, Generic, NFData) | ||
|
|
||
| pattern Null' :: Schema |
There was a problem hiding this comment.
Implementation/naming question: I do not know what is nicer. What do you think?
- Name the type constructors that accept a logical type parameter
Bool,Int, etc and name the patternsBool',Int'etc. (This is what you have now) - Name the patterns
Bool,Intetc, and name the type constructors that accept a logical type parameterBool',Int'etc.
I am leaning somewhat towards the second option, because it would make it easier for library users to upgrade their code. Also, if you were to introduce a custom logical type as a library user, then adding a simple pattern Color = S.String' (LogicalTypeString "color") would be the only place you'd use the type constructor version with a '.
Qqwy
left a comment
Qqwy
left a comment
There was a problem hiding this comment.
There is this big open question I have about schema resolution. Unfortunately, I was unable to find good sources on this question, maybe we need to open the code of other languages' implementations to check.
Other than that, there are two small nits you might want to take a look at.
Nice work Yorick! 🫶
2 participants
Based upon the changes here:
This adds support for unknown logical types to all remaining types (except named and union where it doesn't make sense). There's no known logical types for these, so we're using
LogicalType Void.Please note: We're in a public repo here.
For our (current) purposes, we just needed them for
Records. Did the rest to keep things consistent.