-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-38268] Fix incorrect serialization of VARIANT type FLOAT #27017
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
base: master
Are you sure you want to change the base?
Conversation
51b4625
to
3f935d8
Compare
3f935d8
to
84f8024
Compare
checkCapacity(1 + 4); | ||
writeBuffer[writePos++] = primitiveHeader(FLOAT); | ||
writeLong(writeBuffer, writePos, Float.floatToIntBits(f), 8); | ||
writeLong(writeBuffer, writePos, Float.floatToIntBits(f), 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something? Long is 8 bytes and it is a writeLong. Could it be that checkCapacity(1 + 4);
is incorrect and it should be checkCapacity(1 + 8);
Line 286 is a write long with 8 bytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of writeLong
function might be misleading. The writeLong function does not always write 8 bytes. Its actual functionality is to write Float.floatToIntBits(f)
into writeBuffer[writePos, writePos + numBytes)
. Similarly, you can refer to Line 198
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JinkunLiu ok thanks for the clarification. It agree it is strange/misleading that the method is called writeLong, I would have expected the method to be called writeInt
as it seems to have nothing to do with Longs and is writing an Int. Or writeBytes
to be more generic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hello @Sxnan, What do you think about the name of writeLong
function.
What is the purpose of the change
There is a small bug that
appendFloat
appends 8 bytes, which should be 4 instead. This may cause an ArrayIndexOutOfBoundsException when the buffer doesn't have enough capacity.The page url is https://issues.apache.org/jira/browse/FLINK-38268
Brief change log
Verifying this change
Add a new unit test for FLOAT type. It would fail (throw an exception) without the fix.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation