Skip to content
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

AVRO-3219: Add nullable enum type field support in Avro.Reflect #1348

Merged
merged 3 commits into from
Jul 6, 2022

Conversation

RobertIndie
Copy link
Member

@RobertIndie RobertIndie commented Sep 30, 2021

Signed-off-by: Zike Yang zike@apache.org

Jira

Tests

  • My PR adds the following unit tests: TestNullableEnumResolution

Commits

  • My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • This is just a bug fix. No need doc.

Signed-off-by: Zike Yang <ar@armail.top>
@github-actions github-actions bot added the C# label Sep 30, 2021
@RyanSkraba
Copy link
Contributor

Re: @blachniet @zcsizmadia @KyleSchoonover

This is marked for release 1.11.1 -- would it be possible to take some time to review?

Copy link
Contributor

@KalleOlaviNiemitalo KalleOlaviNiemitalo left a comment

Choose a reason for hiding this comment

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

I don't use Avro.Reflect myself, but I see how this change can improve support for nullable enums, and I don't think this can make anything worse.
The changes in LoadClassCache don't do anything about union schemas with nullable non-enum value types, or non-nullable enum types, but lang/csharp/src/apache/test/Reflect/TestFromAvroProject.cs has tests covering those and GitHub actions don't show any failure.

I approve this but recommend one optimization.

lang/csharp/src/apache/main/Reflect/ClassCache.cs Outdated Show resolved Hide resolved
@KalleOlaviNiemitalo
Copy link
Contributor

KalleOlaviNiemitalo commented Jul 3, 2022

Please indicate in the PR title that this is specifically for Avro.Reflect.

@RyanSkraba
Copy link
Contributor

Thanks for the review @KalleOlaviNiemitalo .

@RobertIndie If you can address the comments, we can probably get this into 1.11.1!

@RobertIndie
Copy link
Member Author

If you can address the comments, we can probably get this into 1.11.1!

Great. I will do it today.

Signed-off-by: Zike Yang <zike@apache.org>
@RobertIndie
Copy link
Member Author

Thanks for your review. PTAL again. @KalleOlaviNiemitalo @RyanSkraba

@KalleOlaviNiemitalo
Copy link
Contributor

The C# code looks OK

@RobertIndie RobertIndie changed the title AVRO-3219: Add nullable enum type field support. AVRO-3219: Add nullable enum type field support in Avro.Reflect Jul 6, 2022
Signed-off-by: Zike Yang <zike@apache.org>
@KalleOlaviNiemitalo
Copy link
Contributor

GitHub actions show that tests passed. I think this is ready to be merged. @RyanSkraba

@RyanSkraba
Copy link
Contributor

Thanks very much for the contribution @RobertIndie and the review @KalleOlaviNiemitalo

@RyanSkraba RyanSkraba merged commit 2a46a61 into apache:master Jul 6, 2022
RyanSkraba pushed a commit that referenced this pull request Jul 6, 2022
* AVRO-3219: Add nullable enum type field support.

Signed-off-by: Zike Yang <ar@armail.top>

* Apply comments.

Signed-off-by: Zike Yang <zike@apache.org>

* Fix mistakenly removed the enum type check.

Signed-off-by: Zike Yang <zike@apache.org>
@RobertIndie RobertIndie deleted the null-enum branch July 7, 2022 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants