You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by Chris Hinds <ch...@bdi.ox.ac.uk> on 2021/08/10 17:11:29 UTC

Re: Protobuf schema provider row functions break on CamelCased field names 

I created an issue for this: https://issues.apache.org/jira/browse/BEAM-12736

I also took a stab at a fix. Would you accept a pull request? Or, I'd be happy to discuss.

Cheers,
Chris.


On 9 Aug 2021, at 21:02, Chris Hinds <ch...@bdi.ox.ac.uk>> wrote:

Haha, it probably shouldn’t!

I’ll have a look.

C.

On 9 Aug 2021, at 19:43, Reuven Lax <re...@google.com>> wrote:


I didn't even realize that the proto compiler allowed camel-case field names! I think the only option would be for someone to add support for camel-case proto field names in Beam.

On Mon, Aug 9, 2021 at 10:57 AM Chris Hinds <ch...@bdi.ox.ac.uk>> wrote:
Hi,

I get an IllegalArgumentException when I call a row function against a proto instance.

SerializableFunction myRowFunction = new ProtoMessageSchema().toRowFunction(new TypeDescriptor<MyDataModel.ProtoPayload>() {});
MyDataModel.ProtoPayload payload = …
Row row = (Row) myRowFunction.apply(payload);

It looks like there is some field name mapping down in ProtoByteBuddyUtils.getProtoGetter() and protoGetterName(...) which assumes proto field names are snake_case, when it tries to map row schema names into proto getter names.

I appreciate proto best practice is to have snake_case field names. Sadly, my proto field names are lowerCamel, and the above lookup fails.

Short of renaming every Proto definition, is there anything I can do?

Cheers,
Chris.

Re: Protobuf schema provider row functions break on CamelCased field names

Posted by Chris Hinds <ch...@bdi.ox.ac.uk>.
I’ve made a bad job of the PR, I’m really sorry. This is my proposed commit:

https://github.com/apache/beam/pull/15306/commits/0f5324321aea01ea035a6e9c46dc80faf1b078f5


On 10 Aug 2021, at 18:15, Reuven Lax <re...@google.com>> wrote:

Definitely happy to look at a PR.

On Tue, Aug 10, 2021 at 10:11 AM Chris Hinds <ch...@bdi.ox.ac.uk>> wrote:
I created an issue for this: https://issues.apache.org/jira/browse/BEAM-12736

I also took a stab at a fix. Would you accept a pull request? Or, I'd be happy to discuss.

Cheers,
Chris.


On 9 Aug 2021, at 21:02, Chris Hinds <ch...@bdi.ox.ac.uk>> wrote:

Haha, it probably shouldn’t!

I’ll have a look.

C.

On 9 Aug 2021, at 19:43, Reuven Lax <re...@google.com>> wrote:


I didn't even realize that the proto compiler allowed camel-case field names! I think the only option would be for someone to add support for camel-case proto field names in Beam.

On Mon, Aug 9, 2021 at 10:57 AM Chris Hinds <ch...@bdi.ox.ac.uk>> wrote:
Hi,

I get an IllegalArgumentException when I call a row function against a proto instance.

SerializableFunction myRowFunction = new ProtoMessageSchema().toRowFunction(new TypeDescriptor<MyDataModel.ProtoPayload>() {});
MyDataModel.ProtoPayload payload = …
Row row = (Row) myRowFunction.apply(payload);

It looks like there is some field name mapping down in ProtoByteBuddyUtils.getProtoGetter() and protoGetterName(...) which assumes proto field names are snake_case, when it tries to map row schema names into proto getter names.

I appreciate proto best practice is to have snake_case field names. Sadly, my proto field names are lowerCamel, and the above lookup fails.

Short of renaming every Proto definition, is there anything I can do?

Cheers,
Chris.

Re: Protobuf schema provider row functions break on CamelCased field names

Posted by Chris Hinds <ch...@bdi.ox.ac.uk>.
In addition to a PR, here’s a work-around for anyone experiencing the same issue:
https://issues.apache.org/jira/browse/BEAM-12736?focusedCommentId=17403123&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17403123

Best wishes,
Chris.


On 10 Aug 2021, at 18:15, Reuven Lax <re...@google.com>> wrote:

Definitely happy to look at a PR.

On Tue, Aug 10, 2021 at 10:11 AM Chris Hinds <ch...@bdi.ox.ac.uk>> wrote:
I created an issue for this: https://issues.apache.org/jira/browse/BEAM-12736

I also took a stab at a fix. Would you accept a pull request? Or, I'd be happy to discuss.

Cheers,
Chris.


On 9 Aug 2021, at 21:02, Chris Hinds <ch...@bdi.ox.ac.uk>> wrote:

Haha, it probably shouldn’t!

I’ll have a look.

C.

On 9 Aug 2021, at 19:43, Reuven Lax <re...@google.com>> wrote:


I didn't even realize that the proto compiler allowed camel-case field names! I think the only option would be for someone to add support for camel-case proto field names in Beam.

On Mon, Aug 9, 2021 at 10:57 AM Chris Hinds <ch...@bdi.ox.ac.uk>> wrote:
Hi,

I get an IllegalArgumentException when I call a row function against a proto instance.

SerializableFunction myRowFunction = new ProtoMessageSchema().toRowFunction(new TypeDescriptor<MyDataModel.ProtoPayload>() {});
MyDataModel.ProtoPayload payload = …
Row row = (Row) myRowFunction.apply(payload);

It looks like there is some field name mapping down in ProtoByteBuddyUtils.getProtoGetter() and protoGetterName(...) which assumes proto field names are snake_case, when it tries to map row schema names into proto getter names.

I appreciate proto best practice is to have snake_case field names. Sadly, my proto field names are lowerCamel, and the above lookup fails.

Short of renaming every Proto definition, is there anything I can do?

Cheers,
Chris.

Re: Protobuf schema provider row functions break on CamelCased field names

Posted by Chris Hinds <ch...@bdi.ox.ac.uk>.
In addition to a PR, here’s a work-around for anyone experiencing the same issue:
https://issues.apache.org/jira/browse/BEAM-12736?focusedCommentId=17403123&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17403123

Best wishes,
Chris.


On 10 Aug 2021, at 18:15, Reuven Lax <re...@google.com>> wrote:

Definitely happy to look at a PR.

On Tue, Aug 10, 2021 at 10:11 AM Chris Hinds <ch...@bdi.ox.ac.uk>> wrote:
I created an issue for this: https://issues.apache.org/jira/browse/BEAM-12736

I also took a stab at a fix. Would you accept a pull request? Or, I'd be happy to discuss.

Cheers,
Chris.


On 9 Aug 2021, at 21:02, Chris Hinds <ch...@bdi.ox.ac.uk>> wrote:

Haha, it probably shouldn’t!

I’ll have a look.

C.

On 9 Aug 2021, at 19:43, Reuven Lax <re...@google.com>> wrote:


I didn't even realize that the proto compiler allowed camel-case field names! I think the only option would be for someone to add support for camel-case proto field names in Beam.

On Mon, Aug 9, 2021 at 10:57 AM Chris Hinds <ch...@bdi.ox.ac.uk>> wrote:
Hi,

I get an IllegalArgumentException when I call a row function against a proto instance.

SerializableFunction myRowFunction = new ProtoMessageSchema().toRowFunction(new TypeDescriptor<MyDataModel.ProtoPayload>() {});
MyDataModel.ProtoPayload payload = …
Row row = (Row) myRowFunction.apply(payload);

It looks like there is some field name mapping down in ProtoByteBuddyUtils.getProtoGetter() and protoGetterName(...) which assumes proto field names are snake_case, when it tries to map row schema names into proto getter names.

I appreciate proto best practice is to have snake_case field names. Sadly, my proto field names are lowerCamel, and the above lookup fails.

Short of renaming every Proto definition, is there anything I can do?

Cheers,
Chris.

Re: Protobuf schema provider row functions break on CamelCased field names

Posted by Reuven Lax <re...@google.com>.
Definitely happy to look at a PR.

On Tue, Aug 10, 2021 at 10:11 AM Chris Hinds <ch...@bdi.ox.ac.uk>
wrote:

> I created an issue for this:
> https://issues.apache.org/jira/browse/BEAM-12736
>
> I also took a stab at a fix. Would you accept a pull request? Or, I'd be
> happy to discuss.
>
> Cheers,
> Chris.
>
>
> On 9 Aug 2021, at 21:02, Chris Hinds <ch...@bdi.ox.ac.uk> wrote:
>
> Haha, it probably shouldn’t!
>
> I’ll have a look.
>
> C.
>
> On 9 Aug 2021, at 19:43, Reuven Lax <re...@google.com> wrote:
>
> 
> I didn't even realize that the proto compiler allowed camel-case field
> names! I think the only option would be for someone to add support for
> camel-case proto field names in Beam.
>
> On Mon, Aug 9, 2021 at 10:57 AM Chris Hinds <ch...@bdi.ox.ac.uk>
> wrote:
>
>> Hi,
>>
>> I get an IllegalArgumentException when I call a row function against a
>> proto instance.
>>
>> SerializableFunction
>> myRowFunction = new ProtoMessageSchema().toRowFunction(new TypeDescriptor<MyDataModel.ProtoPayload>()
>> {});
>> MyDataModel.ProtoPayload payload = …
>> Row row = (Row) myRowFunction.apply(payload);
>>
>> It looks like there is some field name mapping down in
>> ProtoByteBuddyUtils.getProtoGetter() and protoGetterName(...) which assumes
>> proto field names are snake_case, when it tries to map row schema names
>> into proto getter names.
>>
>> I appreciate proto best practice is to have snake_case field names.
>> Sadly, my proto field names are lowerCamel, and the above lookup fails.
>>
>> Short of renaming every Proto definition, is there anything I can do?
>>
>> Cheers,
>> Chris.
>>
>
>

Re: Protobuf schema provider row functions break on CamelCased field names

Posted by Reuven Lax <re...@google.com>.
Definitely happy to look at a PR.

On Tue, Aug 10, 2021 at 10:11 AM Chris Hinds <ch...@bdi.ox.ac.uk>
wrote:

> I created an issue for this:
> https://issues.apache.org/jira/browse/BEAM-12736
>
> I also took a stab at a fix. Would you accept a pull request? Or, I'd be
> happy to discuss.
>
> Cheers,
> Chris.
>
>
> On 9 Aug 2021, at 21:02, Chris Hinds <ch...@bdi.ox.ac.uk> wrote:
>
> Haha, it probably shouldn’t!
>
> I’ll have a look.
>
> C.
>
> On 9 Aug 2021, at 19:43, Reuven Lax <re...@google.com> wrote:
>
> 
> I didn't even realize that the proto compiler allowed camel-case field
> names! I think the only option would be for someone to add support for
> camel-case proto field names in Beam.
>
> On Mon, Aug 9, 2021 at 10:57 AM Chris Hinds <ch...@bdi.ox.ac.uk>
> wrote:
>
>> Hi,
>>
>> I get an IllegalArgumentException when I call a row function against a
>> proto instance.
>>
>> SerializableFunction
>> myRowFunction = new ProtoMessageSchema().toRowFunction(new TypeDescriptor<MyDataModel.ProtoPayload>()
>> {});
>> MyDataModel.ProtoPayload payload = …
>> Row row = (Row) myRowFunction.apply(payload);
>>
>> It looks like there is some field name mapping down in
>> ProtoByteBuddyUtils.getProtoGetter() and protoGetterName(...) which assumes
>> proto field names are snake_case, when it tries to map row schema names
>> into proto getter names.
>>
>> I appreciate proto best practice is to have snake_case field names.
>> Sadly, my proto field names are lowerCamel, and the above lookup fails.
>>
>> Short of renaming every Proto definition, is there anything I can do?
>>
>> Cheers,
>> Chris.
>>
>
>