[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.
This is probably intensional, but where is OBJECT_TYPE?
| 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.
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.
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.
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.
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.
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.
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.
Same comment as with RowWithGetters... should we handle the OneOf logical type as a special case?
There was a problem hiding this comment.
Same response as before.
| .getDeclaredMethods() | ||
| .filter(ElementMatchers.named("toDuration")) | ||
| .getOnly())); | ||
| } else { |
There was a problem hiding this comment.
Don't we need to handle the wrapper types here as well?
There was a problem hiding this comment.
yes! that's why tests are failing, and I'll add that support.
There was a problem hiding this comment.
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.
I would throw RuntimeException with "Not Implemented" for now.
There was a problem hiding this comment.
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.
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.
ProtoRecordSchema seems like a funky name, it's neither Beam nor Proto. Seems like Avro naming.
|
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. |
…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