-
Notifications
You must be signed in to change notification settings - Fork 222
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
Potential narrowing conversion bug in timeMicrosFromNative for time-micros logical type #242
Comments
mihaitodor
added a commit
to mihaitodor/goavro
that referenced
this issue
Mar 25, 2022
Looks like this issue comes from a regression introduced in linkedin#213. Fixes linkedin#242.
mihaitodor
added a commit
to mihaitodor/connect
that referenced
this issue
Apr 5, 2022
When dealing with logical types, we can't serialise the native struct emitted by goavro directly to JSON, since that will discard schema information that `codec.TextualFromNative` uses to produce the expected JSON. Also, the tip version of goavro is required because the fix for this regression linkedin/goavro#242 was merged, but there isn't a tagged release yet. Fixes redpanda-data#1161.
mihaitodor
added a commit
to mihaitodor/connect
that referenced
this issue
Apr 5, 2022
When dealing with logical types, we can't serialise the native struct emitted by goavro directly to JSON, since that will discard schema information that `codec.TextualFromNative` uses to produce the expected JSON. Also, the tip version of goavro is required because the fix for this regression linkedin/goavro#242 was merged, but there isn't a tagged release yet. Fixes redpanda-data#1161.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Given the following schema.avsc
and the following JSON message:
The following
Main.java
programoutputs
{"long_time_micros": 20192000000000}
, while the followingmain.go
programoutputs
{"long_time_micros":{"long.time-micros":1358741504}}
withgithub.com/linkedin/goavro/v2
v2.11.0
.It looks like this is caused by a narrowing conversion here where
duration
should probably be of typeint64
, in which case the cast toint32
should be removed. I'm not knowledgeable enough to read the Java code in detail to see if this is the type they use, but I believe the implementation starts here and the tests for it are here.Note that my go code produces the same output as Java if I remove the
int32
cast intimeMicrosFromNative
for thetime.Duration
case. The tests in this package aren't checking this particular edge case.I'm guessing this is a bug, but please let me know what you think.
The text was updated successfully, but these errors were encountered: