-
Notifications
You must be signed in to change notification settings - Fork 846
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
Add avro-ocf reader codec #1344
Conversation
Thanks for this contribution! I think this implementation will suffer from the logical type serialisation issues that I somewhat fixed via #1198 by forcing the call to Note, however, that introducing |
First of all, I should tell you that this is my first contribution to open source ever, so thanks for your patience and explanations. I didn't fully understand the Message type and its methods, I'll remove goccy, Here's what I'm thinking, changing the implementation to check if the avro codec string contains |
No worries and happy to help out!
I guess it will work, but it feels hackish and the user can't control it. If it's really important to avoid calling |
The issue I was seeing with it was this one - linkedin/goavro#167 Unwanted type literal added to data in textual - the fix for this is pending a merge of a pull request in goavro (Added Two Way Handling for Standard Json - linkedin/goavro#249). I implemented your configuration suggestions, and cleaned up the constructor code a little bit. |
Cool, thanks! I think linkedin/goavro#249 will be merged quite soon (please feel free to upvote it) and then we can call that API both here and in |
I ran
I tried running the script manually but ran into other errors. |
Hey @loicalleyne don't worry the CUE scripts won't change with your MR, this is looking great I just need to put time aside to think through the package layout but at a glance I think it's ready to go. |
Thanks @loicalleyne this looks great! We can take another look once linkedin/goavro#249 is merged. |
Resolves #762
Option to use native encoding/json or github.com/goccy/go-json for JSON marshaling (latter is slightly faster).