[BEAM-12464] Change ProtoSchemaTranslator beam schema creation to match the order for protobufs containing Oneof fields

[reubenvanammers]

Currently, when ProtoSchemaTranslator creates the beam schema from a protobuf definition it always puts the Oneofs at the start of the beam schema due to oneofs always being created first. This means that the order of the fields doesn't match the order of the protobuf definition. As the schema generation is used when converting from beam rows to protobufs, it additionally means that it is impossible to convert from a beam row where the oneof fields are not the first fields in the beamrow.

This change orders the resultant beam schema to match the order in the protobuf definition.

Additionally, this PR updates the test protos so that the beam schema in the oneof tests matches the protobuf definition, as opposed to the beam row having the Oneofs first.

---

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

• Choose reviewer(s) and mention them in a comment (R: @username).
• Format the pull request title like [BEAM-12464] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
• Update CHANGES.md with noteworthy changes.
• If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

ValidatesRunner compliance status (on master branch)

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---			---		---
Java
Python	---			---		---
XLang				---		---

Examples testing status on various runners

Lang	ULR	Dataflow	Flink	Samza	Spark	Twister2
Go	---	---	---	---	---	---	---
Java	---		---	---	---	---	---
Python	---	---	---	---	---	---	---
XLang	---	---	---	---	---	---	---

Post-Commit SDK/Transform Integration Tests Status (on master branch)

Go	Java	Python

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

[reubenvanammers]

Do you have any thoughts about this @reuvenlax ?

[pabloem]

@alexvanboxel would you have time to take a look at this?

[reuvenlax]

My previous comment seems to have been lost. Can you explain the use case where this is necessary? Since oneof fields in Beam are nested rows (unlike in proto), they still won't quite match the proto structure.

[reubenvanammers]

Can you explain the use case where this is necessary?

How I noticed this was when I was trying to convert a Beam Row into a protobuf, with the OneOf not in the first position. During the conversion, the it converts in the order of the converters, which in turn is the order of the generated Schema. Because the OneOf field is first in the generated schema, the first field in the row is attempted to be converted to a proto OneOf, which then fails. Basically this means that Rows containing OneOfs can't be converted to protos, except if all of the OneOfs are at the start of the row.

Obviously there are workarounds (I.e. reordering the beam row before converting to protobuf) , but I believe that the behaviour is surprising enough to warrant changing one way or the other.

[aaltay]

Folks, what do we what do next? Does this require additional discussions?

[reubenvanammers]

Folks, what do we what do next? Does this require additional discussions?

I still believe this change to be worthwhile, considering that currently not all beam schemas containing unions can be converted to protobuf. What would be useful to know whether or not anyone foresees issues arising from this change of behaviour/ anything needs to be added or changed. It'd be useful to get some feedback.

[aaltay]

Folks, what do we what do next? Does this require additional discussions?

I still believe this change to be worthwhile, considering that currently not all beam schemas containing unions can be converted to protobuf. What would be useful to know whether or not anyone foresees issues arising from this change of behaviour/ anything needs to be added or changed. It'd be useful to get some feedback.

You could either add more reviewers or ask for additional comments on the dev@ list.

[pabloem]

@reuvenlax do you have any concerns about this PR?

@ibzib are you able to review a change like this?

[ibzib]

My understanding of the problem here is that OneOfType.create(List, Map) lets the user set arbitrary positions for each of the component types of the OneOfType. So there's no obvious place to put the oneof type in the schema. In the example in the test, the oneof component types are all consecutive (numbers 2-5), so it makes sense to put the oneof at position 2 in the schema. But if they're nonconsecutive, say 2 and 4, it seems kind of arbitrary to put it at position 2 instead of position 4. So we may want to just keep the current behavior and document it, including helpful error messages if we can.

[reubenvanammers]

My understanding of the problem here is that OneOfType.create(List, Map) lets the user set arbitrary positions for each of the component types of the OneOfType. So there's no obvious place to put the oneof type in the schema. In the example in the test, the oneof component types are all consecutive (numbers 2-5), so it makes sense to put the oneof at position 2 in the schema. But if they're nonconsecutive, say 2 and 4, it seems kind of arbitrary to put it at position 2 instead of position 4. So we may want to just keep the current behavior and document it, including helpful error messages if we can.

Hi @ibzib , thanks for having a look!

My understanding is that descriptor.getFields() returns the fields in order that they are in the .proto file, not in order of the field number. This means that in the previous as well as the proposed change, the order of the resultant schema is based on the ordering of the fields in the .proto file, and not the field descriptor number. Because of this, all of the oneof fields are necessarily going to be "clumped", as oneof blocks are contiguous. Therefore, the position of the first field in the oneofblock is a useful and natural location for the entirety of the oneof fields for that particular oneof.

Does that make sense? Happy to write some unit tests for non monotonically increasing proto field descriptor numbers when I've got time if thats the main objection.

[ibzib]

That makes sense now. It's definitely worth adding a unit test where the oneof fields have noncontiguous numbers, and where the oneof field numbers don't line up with the position of the oneof.

[reubenvanammers]

When you have some time @ibzib, mind having a look at this PR again? Added some unit tests with some more protobufs as well as adding some comments.

[ibzib]

Hi Reuben, thanks for updating this PR. I don't have a lot of time at the moment, but I'll be sure to review this when I get the chance.

[ibzib]

Thank you for writing tests! LGTM

[reuvenlax]

I can take a look again.

…

On Fri, Aug 27, 2021 at 2:24 PM Kyle Weaver ***@***.***> wrote: ***@***.**** approved this pull request. Thank you for writing tests! LGTM ------------------------------ In sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoSchemaTranslator.java <#14974 (comment)>: > .withOptions(getFieldOptions(fieldDescriptor))); + } else if (oneOfFieldLocationMap.containsKey(fieldDescriptorNumber)) { + Field oneOfField = oneOfFieldLocationMap.get(fieldDescriptorNumber); + if (oneOfField != null) { In that case, can we save a redundant check like this? Field oneOfField = oneOfFieldLocation.get(fieldDescriptorNumber); if (oneOfField != null) { fields.add(oneOfField); } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14974 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFAYJVNKW6SN5I6XEIZSNVDT677CXANCNFSM46LBRHVA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[reubenvanammers]

Hey @reuvenlax ,
That'd be great to get another set of eyes on it, thanks.

[aaltay]

What is the next step on this PR?

[reubenvanammers]

What is the next step on this PR?

@ibzib appears happy to enough with the PR as it stands. @reuvenlax, do you still want to have a look at the PR, or is it ok to merge? Happy to update the PR if needed.

[aaltay]

@ibzib - if this looks good to you, I would suggest merging it.

[maduris]

hi @reubenvanammers,

Glad you are already looking to the same issue we are facing.

I believe the convert function needs to be also updated to use the identifier number instead of the enum index on line
oneOfConvert has indices using the identifiers from proto definition where oneOf only has enum value.

https://github.com/apache/beam/blob/master/sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoDynamicMessageSchema.java#L731

Something like?

        @Override
        void setOnProtoMessage(Message.Builder message, OneOfType.Value oneOf) {
            int caseIndex = oneOf.getCaseType().getValue();
            int convertIndex =
                logicalType.getCaseEnumType().valueOf(logicalType.getCaseEnumType().getValues().get(caseIndex)).getValue();
            oneOfConvert.get(convertIndex).setOnProtoMessage(message, oneOf.getValue());
        }

[aaltay]

What is the next step on this PR?

[kennknowles]

Pinging @ibzib; do you want a second opinion from someone or is this ready to merge?

[aaltay]

Should this be merged? Closed?

[reubenvanammers]

I feel like this should be merged, unless someone has an objection to it. It's been in approved status limbo for four months now.

[aaltay]

I feel like this should be merged, unless someone has an objection to it. It's been in approved status limbo for four months now.

Got it. I do not have an objection but I also don't know why it was not merged.

@ibzib - since you did the review. Could this be merged? Are there any open discussion points?

[ibzib]

@reubenvanammers can you address this comment? There may not be an issue but I wanted to make sure. Let's add your new test cases to ProtoDynamicMessageSchemaTest as well and see what happens.

hi @reubenvanammers,

Glad you are already looking to the same issue we are facing.

I believe the convert function needs to be also updated to use the identifier number instead of the enum index on line oneOfConvert has indices using the identifiers from proto definition where oneOf only has enum value.

https://github.com/apache/beam/blob/master/sdks/java/extensions/protobuf/src/main/java/org/apache/beam/sdk/extensions/protobuf/ProtoDynamicMessageSchema.java#L731

Something like?

        @Override
        void setOnProtoMessage(Message.Builder message, OneOfType.Value oneOf) {
            int caseIndex = oneOf.getCaseType().getValue();
            int convertIndex =
                logicalType.getCaseEnumType().valueOf(logicalType.getCaseEnumType().getValues().get(caseIndex)).getValue();
            oneOfConvert.get(convertIndex).setOnProtoMessage(message, oneOf.getValue());
        }

[aaltay]

@reubenvanammers - Could you respond to the open comment?

[aaltay]

@reubenvanammers - is this ready for another round of review?

[ibzib]

LGTM - I'll merge this once I can get java precommit to pass

[ibzib]

Run Java PreCommit