scwhittle commented on code in PR #32399:
URL: https://github.com/apache/beam/pull/32399#discussion_r1760790828
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/logicaltypes/OneOfType.java:
##########
@@ -103,12 +100,12 @@ public String getIdentifier() {
@Override
public FieldType getArgumentType() {
- return FieldType.BYTES;
+ return FieldType.row(oneOfSchema);
}
@Override
- public byte[] getArgument() {
Review Comment:
The previous behavior seems incorrect as proto serialization is not
necessarily deterministic and the serialized proto equality doesn't match
schema equality logic.
But I think that you are correct that this would break schema update
compatability because LogicalType base class appears to be serializable.
@robertwb Any ideas on how to best proceed? We could
1. keep OneOf as-is and just document that it's equality isn't guaranteed to
be correct due to proto serialization and clear the uuid before serializing the
proto.
2. add a new type with correct semantics and deprecate this one
3. try to integrate w/ update compatibility version but that seems difficult
given schemas are created all over. We coudl have some static method we call
that modifies static state instead of plumbing the option everywhere.
getArgument base is to return an Object so we can have single class support
both types based upon the configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
