Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Avro] Small improvements #277

Closed
MichalFoksa opened this issue May 16, 2021 · 7 comments
Closed

[Avro] Small improvements #277

MichalFoksa opened this issue May 16, 2021 · 7 comments

Comments

@MichalFoksa
Copy link
Contributor

MichalFoksa commented May 16, 2021

Hi @cowtowncoder,

Here I have few points to improve Avro support inspired by t is inspired by #132 (comment) and also your comment #132 (comment) .

  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 [Avro] Add logicalType support for some java.time types; add AvroJavaTimeModule for native ser/deser #283

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

    Implemented in 2.13 by [Avro] Add logicalType support for some java.time types; add AvroJavaTimeModule for native ser/deser #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 [Avro] Ease future customization by unifying place where a new visitor is created. #292

  4. Rename VisitorFormatWrapperImpl to AvroSchemaVisitorWrapper - or other, better name. Current one is confusing. See [Avro] Rename VisitorFormatWrapperImpl to AvroFormatVisitorWrapper in 2.14 or 3.0 #294

    Will be implemented by [Avro] Rename VisitorFormatWrapperImpl to AvroFormatVisitorWrapper in 2.14 or 3.0 #294

What do you think?
Michal

@cowtowncoder
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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!

@MichalFoksa MichalFoksa changed the title Small improvements to Avro support. [Avro] Small improvements May 22, 2021
@cowtowncoder
Copy link
Member

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
Copy link
Contributor Author

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

@MichalFoksa
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants