avro-kotlin / avro4k

Avro format support for Kotlin
Apache License 2.0
198 stars 37 forks source link

feat: encode/decode anything as root value #161

Closed Chuckame closed 10 months ago

Chuckame commented 1 year ago

This PR needed a big refactor to handle easily

147 partially done (encoder & decoder only). We still have one use case of reflection in schema generation, but will be done into another PR

Chuckame commented 1 year ago

Hey @thake , I'll leave some comments to let you focus or understand some specificities. I tried my best to touch on encoders classes, then decoders classes, then schemas classes.

I don't really know how to split this mess since a lot of things permits the new features together.

Except the encode everything in root, I'll put it into another commit (or PR, but it will need this PR to be merged...)

Chuckame commented 1 year ago

Next step: add some tests for the new features, and implement decode stuff

thake commented 1 year ago

Thanks, @Chuckame! I will look at it as soon as it is ready to merge if this is ok for you. I'm working on a PoC for encoding/decoding without avro library. I'll push a branch of it soon.

Chuckame commented 1 year ago

Cool @thake ! By the way, the work I've did on the encoder/decoder classes will help a lot to really easily encode and decode without using apache library, I would advice you to have a look, it's a common structure when doing kotlinx serialization 🚀

Chuckame commented 1 year ago

@thake It's ready for review. FYI, I will scope this PR only for encoders & decoders about the refactoring. But the main features are covered (read the PR description, there is all the fixes). Schemas are already handling everything as root, but with some reflection and weird stuff with custom serial descriptor. I have the solution (WIP), I'll create another PR !

Hope you'll appreciate this work 😄

btw, I won't have the time this 2 next days to make the benchmark, but it would be interesting. I think there is no real big improvement, but a lot of little things, especially the array/map allocation that is better, and naming that should be much less cpu intensive... Let's see !

thake commented 1 year ago

Just to let you know about how I work with GitHub PRs so that we don't have a misunderstanding: If I have submitted a review result, I won't look at the PR again until you "Re-request review".

So whenever you feel ready, just re-request my review :rocket:

Chuckame commented 1 year ago

Ok, good point 👍 I'm currently taking a pause about avro4k, I'll continue working on it in 1-2 weeks 😄

Chuckame commented 10 months ago

There is too much changes, I'll do some other atomic PRs instead of this big-bang PR

thake commented 10 months ago

Let's consolidate our efforts on this topic, because https://github.com/avro-kotlin/avro4k/tree/way-to-multiplatform also includes a fix for this problem.

Chuckame commented 10 months ago

I think first we should work to improve performance and readability as currently it's very difficult to add feature (kotlinx serialization framework not really well used), then maybe removing the avro lib dependency, and thinking about multiplatform.