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-3126: Add support for java records #1680

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ashley-taylor
Copy link

@ashley-taylor ashley-taylor commented May 5, 2022

To implement this feature, I used a CustomEncoding. Added two new methods to pass the DatumReader/DatumWriter, allowing other CustomEncoding to not have to define an entire custom schema but use a hybrid approach.

With this, I also change AvroEncode to be able to specify at a class level. Feels generically helpful to be able to instrument the class instead of all fields referencing the class.

Used reflection to read the isRecord method from the class class so that it will work on older versions of java.

For records, fields can not be modified or accessed with unsafe offsets reads. Instead, the constructor must be invoked, and reads must be done using either method/field reflection.

To identify the appropriate constructor, I use the order of the fields. Since it is a record and can not be extended, reading the direct fields from the record class is safe.

Make sure you have checked all steps below.

Jira

  • My PR addresses the following AVRO-3126 issues

Tests

  • My PR adds tests
  • CustomEncoder specified at the class Level
  • CustomEncoder specified in an interface or superclass
  • CustomEncoder using the Record encoder
  • Can not test actual java record logic as requires java 17 only test package.

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

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain Javadoc that explain what it does

@github-actions github-actions bot added the Java Pull Requests for Java binding label May 5, 2022
@ashley-taylor ashley-taylor marked this pull request as ready for review May 25, 2022 01:33
@ashley-taylor
Copy link
Author

PR is ready for review.
I cannot test the record logic directly with supporting java 8.
One way I can see doing this is to add another maven module that has java 17 tests only, which is skipped for lower versions. I'm happy to add this module, but I want to ensure I'm not missing something obvious before doing that.
If PR is acceptable, I will squash the commits into a single commit.

@jklamer
Copy link
Contributor

jklamer commented May 25, 2022

I would love the testing in java 17 module if nothing else but to see some examples of the workflow/use. Lots of really cool stuff here that Im still trying to get my head around!

@ashley-taylor
Copy link
Author

@jklamer I have added a java17 module.
I have updated the GitHub workflow to invoke the module hopefully. But unfortunately, PR doesn't currently have the approval to run them.

Also built on top of this branch, I have added polymorphic support.
https://github.com/ashley-taylor/avro/compare/master...ashley-taylor:polymorphic?expand=1
Which I plan on opening if this one gets accepted.

@martin-g
Copy link
Member

I've approved the workflow run!

The new CI related code could be simplified. I will do it soon!

I'll let the Java developers to comment on the Java changes!

@ashley-taylor
Copy link
Author

@martin-g I noticed a bug in how I had conditionally included the java17 module with a profile.
The version of the java17 module would not increment automatically when performing a release, causing a manual headache.

I changed it always to be included but have the compile and test plugins disabled so that it won't complain about the version of java.
Annoyingly I can't put any logic in the maven properties, so the property to enable it has to be named backwards disableJava17.
This way, the module will be included for lining and release purposes but not actually run the code unless configured to set the flag.
Changed the build.sh script to have a new option to run in java17 mode and add an expression to execute one of the two test steps conditionally.

@martin-g
Copy link
Member

We should not merge this PR for branch-1.11 because it will definitely cause problems with the release!
I guess we will have to use JDK 17 for the release process and -release 8 for all old modules. Actually no, this is a module with just test cases, so it does not need to be released at all!

Have extended the functionality of the CustomEncoding to implement this
AvroEncode annotation can now also be added to a class

adding a new module to write tests that require java 17 features
Added module that doesn't compile or test by default
@ashley-taylor
Copy link
Author

@martin-g @jklamer
Have added some more tests and squashed the commits.
Is there anything I can do to assist in getting this PR merged?

Thanks

@martin-g
Copy link
Member

martin-g commented Jun 8, 2022

@ashley-taylor The PR should be reviewed and merged by a committer who knows better the Java SDK.
I review most of the PRs for all SDKs but my main expertise is in the Rust SDK.

@RyanSkraba
Copy link
Contributor

Hello! Thanks for your contribution and for your patience! I've set this fix version for the next major release of Avro (1.12.0, later this year). I know record support would be a great feature to get in.

Do you have a JIRA account so we can assign this JIRA to you and ensure that it's properly taken care of?

@ashley-taylor
Copy link
Author

Hi @RyanSkraba thanks for the update

Have added a comment on the Jira ticket

I also have another Branch to address AVRO-1568 Polymorphism that I have built on top of this branch. Looking to open that PR after this one.

@martin-g
Copy link
Member

martin-g commented Jun 9, 2022

Do you have a JIRA account so we can assign this JIRA to you and ensure that it's properly taken care of?

I tried to add Ashley to the Contributors role in JIRA but there are 5 JIRA users with that (display) name and I am not sure which one to add.

@ashley-taylor
Copy link
Author

Do you have a JIRA account so we can assign this JIRA to you and ensure that it's properly taken care of?

I tried to add Ashley to the Contributors role in JIRA but there are 5 JIRA users with that (display) name and I am not sure which one to add.

ashley-taylor

is my jira username

@martin-g
Copy link
Member

martin-g commented Jun 9, 2022

I know but JIRA offers me 5 users with "Ashley Taylor" as display name when I enter this id:
ashley-taylor-avro-contributor

@ashley-taylor
Copy link
Author

I know but JIRA offers me 5 users with "Ashley Taylor" as display name when I enter this id: ashley-taylor-avro-contributor

I don't know how you did it. But have correctly assigned to the correct one. My name is clearly too common.

@martin-g
Copy link
Member

martin-g commented Jun 9, 2022

I did nothing! :-)
Someone else did it.

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.

Looks like a great addition.
I like the care given to the combination of records, wrapped classes, inheritance and custom encodings. That could otherwise cause bugs.

@RyanSkraba RyanSkraba changed the title AVRO-3126 Add support for java records AVRO-3126: Add support for java records Jul 4, 2022
@ashley-taylor
Copy link
Author

anything I can do to help this get merged?

@martin-g
Copy link
Member

martin-g commented Oct 5, 2022

@ashley-taylor The JIRA ticket has Fix version set to 1.12.0, so it will be reviewed and merged before the release!
I cannot say when 1.12.0 will be released though!

@carl-mastrangelo
Copy link

I would like also like to express my interest in this getting merged. We use Avro with our Apache Beam classes, and being able to use Record classes would make things more seamless.

@ashley-taylor
Copy link
Author

I have made some additional Java 17 improvements that are based on this and I wondering if I should add this branch.
If the classes are sealed it will automatically add the equivalent of the @union annotation, so polymorphism is automatic.

https://github.com/ashley-taylor/avro/pull/1/files

@RyanSkraba RyanSkraba self-requested a review June 29, 2023 11:09
@JoeryH
Copy link

JoeryH commented Jan 4, 2024

Just wanted to share that record support for Avro would be really nice to have!

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
7 participants