AVRO-1891: Fix specific nested logical types

[rdblue]

This changes the datum readers to use getConversionFor and getConversionByClass instance methods that delegate to the data models. This allows specific to use a set of conversions determined by the current record class to pick up the conversions that were present when it was compiled.

[yibing-shi]

peekLast() could return null. We need to check it to avoid NPE.

[yibing-shi]

And also, we would need to change the datum writers to support the conversions, do we?

[rdblue]

Yes, this just shows how I think we could fix the problem. We still need to finish the write side and add tests.

[jeremyvdw]

Hi,
Any updates on this?

Thanks

[simplesteph]

Hi @rdblue and @yibing-shi , any updates on this? is it the same as https://stackoverflow.com/questions/45581437/how-to-specify-converter-for-default-value-in-avro-union-logical-type-fields ?

[nandorKollar]

There are missing imports from the template:

import java.util.ArrayList;
import java.util.Collection;
import org.apache.avro.Conversion;

What is CONVERSIONS used for? Is this needed?

[nandorKollar]

@rdblue I think think the template is not yet processed correctly. It seems, that for union schema, SpecificCompiler#hasLogicalTypeField returns false even when the union has logical types, due to this the conversion fields are not included in the compiled class. Should it instead check each type in the union? Something like this:

public boolean hasLogicalTypeField(Schema schema) {
for (Schema.Field field : schema.getFields()) {
if (field.schema().getType() == Schema.Type.UNION) {
for (Schema unionSchema : field.schema().getTypes()) {
if (unionSchema.getLogicalType() != null) {
return true;
}
}
} else {
if (field.schema().getLogicalType() != null) {
return true;
}
}
}
return false;
}

[busbey]

@rdblue are you still working on this?

[Igosuki]

This won't work for unions, because a union field may have multiple logical types by definition...

[wheelerlaw]

What happened to this? This seems pretty major, because without it, unions with built-in logical types are broken.

[rdblue]

@wheelerlaw, feel free to pick this up. I'll help review the changes.

[lewisdawson]

Is there still any intention of finishing & merging this PR? I ran into this today when trying to serialize avro data to a Kafka stream.

[katrinsko]

I submitted a new PR today, #329, with a different solution approach to AVRO-1891. Feedback appreciated!

[katrinsko]

Hey @lewisdawson, there is a workaround for the current version that might help you. It involves implementing your own Avro serializer (or slightly modifying the one you're using, I did that with Confluent's). The key is to explicitly add the right conversions for logical types to SpecificData when creating the DatumWriter and DatumReader. If you still need help, create a post on StackOverflow and we'll take it over there.

[lazyval]

@scatrin can you follow up on that? it seems like the problem is still ongoing despite the effort from Avro community

I wonder if it's possible to do in user space, without building the custom version of avro library

[katrinsko]

@lazyval The workaround that I did applies to a setting with Kafka and Confluent's Schema Registry. The trick is to modify the serializer/deserializers (from Confluent) to explicitly register the needed conversions when creating the SpecificDatumWriter/Reader. (for instance here). SpecificDatumWriter has a getter for the SpecificData instance and SpecificData (via its superclass) has a method for registering logical type conversions. The cool thing is that the conversion class does not have to be part of Avro, you can build your own if you want to. Then just make sure that the modified classes are in the classpath when serializing/deserializing and that it is referenced properly in the Kafka settings.

[dkulp]

I merged #329, is this not needed now?

[katrinsko]

@dkulp I looked into the possibility of completing this PR first but ended up choosing a different solution and submitting it in #329 instead. With #329 merged I would say that this one is obsolete.