-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
[FLINK-35406] Use inner serializer when casting RAW type to BINARY or… #24818
base: master
Are you sure you want to change the base?
Conversation
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.
That is a nasty bug. Thanks for fixing it @docete. I just had a tiny code improvement suggestion. Otherwise LGTM.
...e-planner/src/test/java/org/apache/flink/table/planner/functions/CastFunctionMiscITCase.java
Show resolved
Hide resolved
bb62c81
to
df83a22
Compare
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.
One final question before merging.
@@ -62,7 +62,7 @@ public boolean canFail(LogicalType inputLogicalType, LogicalType targetLogicalTy | |||
// new behavior | |||
isNull$290 = isNull$289; | |||
if (!isNull$290) { | |||
byte[] deserializedByteArray$76 = result$289.toBytes(typeSerializer$292); | |||
byte[] deserializedByteArray$76 = result$289.toBytes(typeSerializer$292.getInnerSerializer()); |
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.
Why do we actually need the call to getInnerSerializer()
during runtime shouldn't we simply use this serializer as the typeSerializer$292
for code generation? Or does the implementation not allow this?
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.
Why do we actually need the call to
getInnerSerializer()
during runtime shouldn't we simply use this serializer as thetypeSerializer$292
for code generation? Or does the implementation not allow this?
I think the RAW type should bind with RawValueDataSerializer, and use getInnerSerializer()
in se/de phase. Follow the same pattern also make the Generated code more clear. See: AbstractBinaryWriter::writeRawValue
public void writeRawValue(
int pos, RawValueData<?> input, RawValueDataSerializer<?> serializer) {
TypeSerializer innerSerializer = serializer.getInnerSerializer();
// RawValueData only has one implementation which is BinaryRawValueData
BinaryRawValueData rawValue = (BinaryRawValueData) input;
rawValue.ensureMaterialized(innerSerializer);
writeSegmentsToVarLenPart(
pos, rawValue.getSegments(), rawValue.getOffset(), rawValue.getSizeInBytes());
}
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.
BTW, modify the code generator make things more complicated
… STRING in cast rules
What is the purpose of the change
This pull request fix the wrong behaviour in casting RAW to BINARY or STRING.
The generated code in RawToStringCastRule and RawToBinaryCastRule use
BinaryRawValueData::toBytes and BinaryRawValueData::toObject to convert
RawValueData(to java object or byte array), which should use inner serializer
instead of RawValueDataSerializer.
Brief change log
Verifying this change
This change is convered by new test cases in CastFunctionMiscITCase and CastFunctionMiscLegacyITCase
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation