-
Notifications
You must be signed in to change notification settings - Fork 1.7k
AVRO-4133: Support default enum value in Protobuf to Avro #3367
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
Conversation
martin-g
left a comment
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.
|
@martin-g Could you also merge this? |
|
@zenfenan Would you mind add an extra test case to demonstrate this change? It seems that at the moment nothing tests enum schema conversions, I think |
opwvhk
left a comment
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.
The change itself looks good, but it lacks a test case.
Preferably with the link @martin-g provided: https://protobuf.dev/programming-guides/proto3/#default
lang/java/protobuf/src/test/java/org/apache/avro/protobuf/noopt/TestProto3.java
Dismissed
Show dismissed
Hide dismissed
lang/java/protobuf/src/test/java/org/apache/avro/protobuf/noopt/TestProto3.java
Dismissed
Show dismissed
Hide dismissed
lang/java/protobuf/src/test/java/org/apache/avro/protobuf/noopt/TestProto3.java
Dismissed
Show dismissed
Hide dismissed
lang/java/protobuf/src/test/java/org/apache/avro/protobuf/noopt/TestProto3.java
Dismissed
Show dismissed
Hide dismissed
lang/java/protobuf/src/test/java/org/apache/avro/protobuf/noopt/TestProto3.java
Dismissed
Show dismissed
Hide dismissed
lang/java/protobuf/src/test/java/org/apache/avro/protobuf/noopt/TestProto3.java
Dismissed
Show dismissed
Hide dismissed
lang/java/protobuf/src/test/java/org/apache/avro/protobuf/noopt/TestProto3.java
Dismissed
Show dismissed
Hide dismissed
lang/java/protobuf/src/test/java/org/apache/avro/protobuf/noopt/TestProto3.java
Dismissed
Show dismissed
Hide dismissed
lang/java/protobuf/src/test/java/org/apache/avro/protobuf/noopt/TestProto3.java
Dismissed
Show dismissed
Hide dismissed
|
Thanks for your reviews! I realised that you can actually have default value in your protobuf schema until proto2. It is only removed in proto3. Refer:
So I updated the code to handle that, and also added a test (with new proto3 schema). Let me know if the commits need to be squashed into a single commit. BTW, the CodeQL violations are on the protobuf generated code. I couldn't find any exclusion config in the project. Let me know if this needs to be handled or not! |
|
I like this test. Especially the overridden default. What i don't see, however, is the field default values in the first two cases. An opinion on the CodeQL comments on generated code: as its generated code, by a generator outside or control, I place little to no importance to these style comments. I'd mark them as "won't fix". |
|
@opwvhk I am unable to mark the CodeQL warnings as "Won't fix". I also don't have the permission to merge. Possibly because I am not part of the committers list for this repo. If there is nothing else pending, could you resolve the CodeQL warnings and also merge the PR? Thanks! |
* [AVRO-4133] Support default enum value in Protobuf to Avro * [AVRO-4133] Add unit test for proto schema enum default * [AVRO-4133] Exclude generated test proto code --------- Co-authored-by: sivaprasanna <sivaprasanna@sivaprasannas-MacBook-Air.local>
What is the purpose of the change
This PR creates enum Schema with the default symbol when converting Protobuf descriptor to Avro schema.
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Documentation