AVRO-4322: [Java] Only apply java-class annotations to SpecificData models - #3917
Conversation
There was a problem hiding this comment.
Pull request overview
This PR restores Avro’s pre-1.12.1 behavior by ensuring java-class / java-key-class schema annotations are applied only when using the SpecificData model, not GenericData, while adding/adjusting tests to lock in that behavior (including class-loading validation under ClassSecurityValidator).
Changes:
- Restricts
FastReaderBuilder’s application ofSpecificData.CLASS_PROP/SpecificData.KEY_CLASS_PROPtoSpecificDatamodel instances. - Expands
FastReaderBuilderJavaClassTestto validate both “Generic ignores / Specific applies” behavior for string fields and map keys. - Updates the class-loading security test to use a
SpecificDatamodel and shared round-trip helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lang/java/avro/src/main/java/org/apache/avro/io/FastReaderBuilder.java | Gates java-class / java-key-class transformations to SpecificData model usage and slightly refactors map key reader creation. |
| lang/java/avro/src/test/java/org/apache/avro/io/FastReaderBuilderJavaClassTest.java | Adds comprehensive regression tests asserting Generic vs Specific behavior for CLASS_PROP and KEY_CLASS_PROP. |
| lang/java/avro/src/test/java/org/apache/avro/io/TestFastReaderBuilderClassLoading.java | Aligns the security/class-loading test with the new “Specific-only” behavior and reuses the shared round-trip helper. |
Suppressed comments (4)
lang/java/avro/src/test/java/org/apache/avro/io/FastReaderBuilderJavaClassTest.java:120
- This test relies on the global default for fast-reader enablement (system property + GenericData singleton). If fast-reader is disabled in the test JVM, this won’t exercise FastReaderBuilder and may not catch regressions. Prefer using a fresh model instance with fast-reader explicitly enabled.
GenericRecord result = roundTrip(RECORD_WITH_CLASS_PROP, GenericData.get());
lang/java/avro/src/test/java/org/apache/avro/io/FastReaderBuilderJavaClassTest.java:152
- This test should explicitly enable fast-reader on a fresh SpecificData instance to ensure it validates the FastReaderBuilder path regardless of JVM/system-property configuration.
GenericRecord result = roundTrip(RECORD_WITH_CLASS_PROP, SpecificData.get());
lang/java/avro/src/test/java/org/apache/avro/io/FastReaderBuilderJavaClassTest.java:169
- This test relies on the global default for fast-reader enablement (system property + GenericData singleton). If fast-reader is disabled in the test JVM, this won’t exercise FastReaderBuilder and may not catch regressions in the fast path. Prefer using a fresh model instance with fast-reader explicitly enabled.
GenericRecord result = roundTrip(RECORD_WITH_MAP_KEY_CLASS_PROP, GenericData.get());
lang/java/avro/src/test/java/org/apache/avro/io/FastReaderBuilderJavaClassTest.java:192
- This test should explicitly enable fast-reader on a fresh SpecificData instance to ensure it validates the FastReaderBuilder path regardless of JVM/system-property configuration.
GenericRecord result = roundTrip(RECORD_WITH_MAP_KEY_CLASS_PROP, SpecificData.get());
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Copilot isn't wrong (except that we are pretty much guaranteed to run with fast-read and classic read because of the pom wrangling we do). I'm going to update this class to test both behaviours to ensure they remain consistent. |
56c688f to
501bd37
Compare
|
I changed my mind about starting a strategy of "testing both" with parameters here - GitHub Actions and ./build.sh test will catch if the behaviours diverge. In order for that to work, we shouldn't setFastReadEnabled(...) at all and use it from the global properties. |
|
Cherry-picked to branch-1.12. |
What is the purpose of the change
When the fastread function was set to be the default in Avro 1.12.1, the behaviour around create GenericData objects changed.
In the past, the GenericData returned standard and reliable "generic" data types that didn't involve casting or reflection, while SpecificData allowed the user to create "specific" java classes (including the
SpecificData.CLASS_PROPandSpecificData.KEY_CLASS_PROPto control the representation of a STRING datum.Currently, fastread changed this behaviour to apply those properties to the GenericData model, which causes tests to fail in Parquet. This restores the original behaviour.
Verifying this change
This change added tests and can be verified by running the
TestFastReaderBuilderJavaClassTest.Documentation