Skip to content

Conversation

@zenfenan
Copy link
Contributor

@zenfenan zenfenan commented May 1, 2025

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

  • Does this pull request introduce a new feature? (yes)
  • If yes, how is the feature documented? (not applicable)

@github-actions github-actions bot added the Java Pull Requests for Java binding label May 1, 2025
@zenfenan zenfenan changed the title [AVRO-4133] Support default enum value in Protobuf to Avro AVRO-4133: Support default enum value in Protobuf to Avro May 1, 2025
Copy link
Member

@martin-g martin-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zenfenan
Copy link
Contributor Author

zenfenan commented May 2, 2025

@martin-g Could you also merge this?

@nandorKollar
Copy link
Contributor

@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 TestProbuf would be a good place for an extra test case. It appears, that we didn't handle default tags in the past either, like the example in test.proto: optional A enum = 16 [default = Z];. Probably we should raise a separate Jira to handle default tags properly.

Copy link
Contributor

@opwvhk opwvhk left a 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

@github-actions github-actions bot added the build label Jun 6, 2025
@zenfenan
Copy link
Contributor Author

zenfenan commented Jun 6, 2025

@nandorKollar & @opwvhk

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!

@zenfenan zenfenan requested a review from opwvhk June 6, 2025 21:56
@opwvhk
Copy link
Contributor

opwvhk commented Jun 8, 2025

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".

@zenfenan
Copy link
Contributor Author

zenfenan commented Jun 9, 2025

@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!

@opwvhk opwvhk merged commit 5f7c2d3 into apache:main Jun 10, 2025
8 checks passed
opwvhk pushed a commit that referenced this pull request Jun 13, 2025
* [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>
(cherry picked from commit 5f7c2d3)
opwvhk pushed a commit to opwvhk/avro that referenced this pull request Sep 5, 2025
* [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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Java Pull Requests for Java binding

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants