-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BEAM-7274] Implement the Protobuf schema provider for compiled protocol buffers #10449
[BEAM-7274] Implement the Protobuf schema provider for compiled protocol buffers #10449
Conversation
@@ -94,7 +94,6 @@ | |||
new ForLoadedType(ReadableInstant.class); | |||
private static final ForLoadedType READABLE_PARTIAL_TYPE = | |||
new ForLoadedType(ReadablePartial.class); | |||
private static final ForLoadedType OBJECT_TYPE = new ForLoadedType(Object.class); |
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.
This is probably intensional, but where is OBJECT_TYPE?
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.
It was unused.
import org.apache.beam.vendor.guava.v26_0_jre.com.google.common.collect.Multimap; | ||
|
||
@Experimental(Kind.SCHEMAS) | ||
public class ProtoRecordSchema extends GetterBasedSchemaProvider { |
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.
Funky naming, looks like spilled over from Avro implementation. Wouldn't ProtoMessageSchema be better?
@@ -122,6 +123,15 @@ private Iterable getIterableValue(FieldType elementType, Object fieldValue) { | |||
cacheKey, i -> getMapValue(type.getMapKeyType(), type.getMapValueType(), map)) | |||
: (T) getMapValue(type.getMapKeyType(), type.getMapValueType(), map); | |||
} else { | |||
if (type.isLogicalType(OneOfType.IDENTIFIER)) { |
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.
Should we handle the oneOf (or UnionType) separately from all other LogicalTypes. I think we decided to leave Enums also out for now. I think it's best that we handle this separate from the Proto PR.
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.
I thought that originally, however it turns out that the only way to make OneOf work with the getters framework is to actually handle it explicitly, otherwise recursive translation fails to work. Happy to pull this out into a separate PR if you think that's better.
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.
My gut feeling says that we should take it separately. Ok, union types will fail, but for now it's only proto specific right? I've added this on my todo list when testing the implementation with our use cases.
OneOfType.Value oneOfValue = (OneOfType.Value) fieldValue; | ||
Object convertedOneOfField = | ||
getValue(oneOfValue.getFieldType(), oneOfValue.getValue(), null); | ||
return (T) |
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.
Will this not generate a different type for each message. If conversion happens here as a user of the getValue you don't have a way to see what the effective type and field was. Why not return the LogicalTypes?
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.
This is because the current semantics of Row.getValue() is to return the underlying type and you're expected to call Row.getLogicalTypeValue() to get the LogicalType value. It's debatable whether that's the best solution for Row, but there are arguments both ways and I didn't want to change those semantics here.
@@ -127,6 +123,22 @@ public T apply(Row row) { | |||
valueType, | |||
typeFactory); | |||
} else { | |||
if (type.getTypeName().isLogicalType() | |||
&& OneOfType.IDENTIFIER.equals(type.getLogicalType().getIdentifier())) { |
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.
Same comment as with RowWithGetters... should we handle the OneOf logical type as a special case?
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.
Same response as before.
.getDeclaredMethods() | ||
.filter(ElementMatchers.named("toDuration")) | ||
.getOnly())); | ||
} else { |
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.
Don't we need to handle the wrapper types here as well?
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.
yes! that's why tests are failing, and I'll add that support.
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.
yes! that's why tests are failing, and I'll add that support.
|
||
@Override | ||
public List<FieldValueGetter> fieldValueGetters(Class<?> targetClass, Schema schema) { | ||
return null; |
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.
I would throw RuntimeException with "Not Implemented" for now.
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.
Actually, I meant to remove this file from this PR - it belongs in the next PR. I'll remove it.
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.
Actually, I meant to remove this file from this PR - it belongs in the next PR. I'll remove it.
import org.apache.beam.vendor.guava.v26_0_jre.com.google.common.collect.Multimap; | ||
|
||
@Experimental(Kind.SCHEMAS) | ||
public class ProtoRecordSchema extends GetterBasedSchemaProvider { |
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.
ProtoRecordSchema seems like a funky name, it's neither Beam nor Proto. Seems like Avro naming.
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.
Changed.
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.
Maybe remove the OneOf in the Row (and let's mark it as a known issue). If test are green + spotless I'll give a LGTM.
If I remove the OneOf then tests will start failing, because we won't be
able to properly support the codegen for OneOf (basically we would then be
removing OneOf support in protobuf as well).
…On Mon, Dec 23, 2019 at 2:39 AM Alex Van Boxel ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Maybe remove the OneOf in the Row (and let's mark it as a known issue). If
test are green + spotless I'll give a LGTM.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#10449?email_source=notifications&email_token=AFAYJVIB3HZJN2QCPGPQJP3Q2CWQ5A5CNFSM4J6IM3LKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQCJFUA#pullrequestreview-335844048>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFAYJVOORB4IBJV54TEVGKLQ2CWQ5ANCNFSM4J6IM3LA>
.
|
OK, let's keep it in then. Better to tackle it later then. Spotless and Java tests are failing though. |
@alexvanboxel support for wrapper types added, and all tests are now green. |
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.
LGTM, only thing I think needs revision later on is the OneOf. I'll need to play with it to make a good assessment.
…chema provider for compiled protocol buffers
…chema provider for compiled protocol buffers
Since compiled protocol buffers have efficient generated getters and builders, we reuse the ByteBuddy framework to create lazy rows that delegate to those methods.
Support for DynamicMessage is in a followon PR.
R: @alexvanboxel