add support for maps with list values #86
Conversation
|
First of all THX for you contribution. Can you maybe add to this and provide the following?
Many THX again for your efforts and looking into this @sfmontyo |
|
So I've modified the RecordConverterTest to test for maps with lists of ints or strings in addition to the other data types it was checking and the tests pass. |
|
Ok I see. Honestly by reading the AVRO spec back then when I implemented this I wasn't aware that this is supported. Because the docs actually seem to not mention what you suggest here. Irrespective of your current implementation, have you tried to write an AVRO schema file containing a map which in turn has an array as values? Also what about the other complex types of AVRO, can they also be used as value types in maps? Happy to hear your thoughts! |
|
Ah yes. We have data being shipped across an avro topic and consumed by the standard elasticsearch connector which blew up when we tried to use the mongo connector. The avro schema was handled fine by the standard elasticsearch connector just not by the mongo one. As far as I know, the only requirement for maps is that their key is a string. |
|
Great that you found this! Coming back to my other question: Also what about the other complex types of AVRO, can they also be used as value types in maps? And it would be nice - like you assumed anyway - if you can adapt the README in the section near the top with the sample AVRO schema to reflect this change e.g. by adding a map with list of string/int respectively. If I understood your PR correctly the current behaviour is not affected and still works e.g. maps with string keys and any primitive type, right? Looking forward to get this merged after making some final checks and maybe small adaptions in the tests. THX again for your contribution! |
|
Here is our avro spec for the class. |
|
So I've modified the example to have an optional map field that if present, will be a Map<String,List> . Oh and btw, thank you for maintaining this connector! |
|
btw - my reading of the spec is that the values can be any valid schema including primitives, arrays, maps and structs. My changes support arrays, thus allowing map values to be primitives, structs or arrays. Looking at the code base, it doesn't look like it would handle a map having values that are also maps. |
sounds good!
you think it would make sense to support maps in maps? I haven't seen any use case for it so far which is why this whole issue never popped up until now that you ran into it :) |
|
We don't need maps of maps and given no one has requested it, it doesn't seem of great importance. I only mentioned it as to answer your query what other complex Avro types are supported. That wasn't a request of mine :) |
| @@ -82,6 +84,8 @@ public static void initializeTestData() { | |||
| .build()) | |||
| ) | |||
| .field("mySubDoc2", SchemaBuilder.map(Schema.STRING_SCHEMA, Schema.INT32_SCHEMA).build()) | |||
| .field( "myMapOfStrings", SchemaBuilder.map(Schema.STRING_SCHEMA, SchemaBuilder.array(Schema.STRING_SCHEMA).build()).build()) | |||
There was a problem hiding this comment.
hi again. sorry it took me some time to get back on this...
I'm really looking forward to merging this. Let me suggest one more small aspect which is that I think for consistency reasons of this test we should also add the two maps to the JSON_STRING_1 above at right before the "myBytes" field
see 0c0ad67#diff-ab414b445c07f6db6f112afe2ca89aafL63
could you maybe please do it directly in your branch? then it's the least effort I guess to get it merged :)
|
Ok I modified both the OBJ_MAP_1 and JSON_STRING_1 to have the same fields as OBJ_SCHEMA_1 . |
|
THX @sfmontyo for this contribution. Highly appreciated! As always I'd be happy to learn about your concrete use cases for my project. If possible, please share :) |
Avro has support for maps with lists of strings. I've modified the AvroJsonSchemafulRecordConverter.java to handle this case.