FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
310 stars 133 forks source link

[Avro] Small improvements #277

Closed MichalFoksa closed 3 years ago

MichalFoksa commented 3 years ago

Hi @cowtowncoder,

Here I have few points to improve Avro support inspired by t is inspired by https://github.com/FasterXML/jackson-dataformats-binary/issues/132#issuecomment-827574594 and also your comment https://github.com/FasterXML/jackson-dataformats-binary/issues/132#issuecomment-828043002 .

  1. Simple mapping of few Java date-time types to Avro logicalType on expectIntegerFormat(JavaType type). The mapping works if ObjectMapper is used with JavaTimeModule and WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS is disabled. In this combination JSR310 types are mapped to Avro LONG, only the logical type is missing.

    Implemented in 2.13 by #283

    new ObjectMapper
        .registerModule(new JavaTimeModule())
        .disable(SerializationFeature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS)
    ...
  2. Avro specific time module?

    Implemented in 2.13 by #283

  3. To ease customization by a user I suggest to add protected method VisitorFormatWrapperImpl createVisitorWrapper() into VisitorFormatWrapperImpl which creates and returns a new instance of visitor wrapper. This method would be used on each place where new visitor instance is created in ArrayVisitor, MapVisitor and RecordVisitor.

    By having this in place it is easier to add / modify existing functionality by a user. He just needs to extend existing visitor wrapper and override particular method (just ask me how I know it).

    protected VisitorFormatWrapperImpl createVisitorFormatWrapperImpl() {
        return new VisitorFormatWrapperImpl(_schemas, _provider);
    }

    Implemented in 2.13 by #292

  4. Rename VisitorFormatWrapperImpl to AvroSchemaVisitorWrapper - or other, better name. Current one is confusing. See #294

    Will be implemented by #294

What do you think? Michal

cowtowncoder commented 3 years ago

I think this probably makes sense, although it has been a while since I worked on this module. I think it would be worth bringing up on jackson-dev Google group.

It also seems that we'd really need active maintainer(s) for the Avro format module. There have been a few contributors with good ideas on improvements, but I think there needs to be someone with a full vision of how to develop this module: something similar to how Scala and Kotlin modules work (I help where I can but I am not driving development so to speak).

I can bring this up on the mailing list: you could be one of possible maintainers if that interests you?

MichalFoksa commented 3 years ago

Hmmm, I do not have a vision of how to develop the module. At the moment I only have few points to help with my life.

What authority has jackson-dev group on changes here?

cowtowncoder commented 3 years ago

Jackson-dev is just a discussion group, but it is about the only channel where multiple developers would see messages; not many follow issue tracker.

So asking on that list would just help get some feedback, suggestions & raise awareness. It is not a requirement or anything.

On maintainers: eventually I hope to find a contributor or two who have spent enough time with PRs to have a good overall idea of how things connect and who could then guide PRs. Since this is fully voluntary thing to do, time used for that obviously varies; and at any given time there may or may not be someone who can take care of issues. For Kotlin module there are official 2 maintainers (beyond original author who is not active any more), and one has been much more active than the other. This is fine and I think ideal number of maintainers is probably 2 or 3; small enough to get easy consensus, but more than 1 to get different view points.

cowtowncoder commented 3 years ago

On the issue & PR here: I'll be on a brief vacation starting today, back next week. I will have a look then and hopefully be able to merge changes; I don't have any concerns per se but want to make sure I know all changes that I merge and have no major concerns.

Thank you once again for helping improve this module!

cowtowncoder commented 3 years ago

Can this be closed @MichalFoksa or do you want to keep it open for discussions?

I also noticed (4) on renaming -- I'd be +1 in general, although the usual question is whether it's likely this might break someone somewhere (for 2.x). For Jackson 3.0 renaming obviously possible.

MichalFoksa commented 3 years ago

@cowtowncoder I would keep it for discussion, but if it is inconvenient close it.

MichalFoksa commented 3 years ago

I am closing this issue - all points are either done or follow up issue is created.