reubenvanammers commented on pull request #14974: URL: https://github.com/apache/beam/pull/14974#issuecomment-896694002
> My understanding of the problem here is that [OneOfType.create(List, Map)](https://beam.apache.org/releases/javadoc/2.31.0/org/apache/beam/sdk/schemas/logicaltypes/OneOfType.html#create-java.util.List-java.util.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. -- 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]
