AVRO-4332: [java] Add support for enum default values in IDL serialization - #3822
AVRO-4332: [java] Add support for enum default values in IDL serialization#3822prygunov wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for emitting enum default values when serializing schemas to Avro IDL, with accompanying tests to validate the output.
Changes:
- Update IDL serialization to append
= <default>;after enum definitions when an enum default is present. - Add a unit test to verify enum defaults are (and are not) written to IDL output.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lang/java/idl/src/main/java/org/apache/avro/idl/IdlUtils.java | Writes enum defaults into the generated IDL output when present on the schema. |
| lang/java/idl/src/test/java/org/apache/avro/idl/IdlUtilsTest.java | Adds coverage to assert enum default serialization behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@RyanSkraba can you take this to the next RC?) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
lang/java/idl/src/test/java/org/apache/avro/idl/IdlUtilsTest.java:117
- The PR description says the test also verifies that an enum schema without a default value is unaffected (no "=") but the current test only covers the "with default" case. To avoid regressions (e.g., always emitting "= ..." or a stray "="), add a negative assertion (or a second test) for an enum created without a default, or update the PR description to match what is actually tested.
@Test
public void enumDefaultIsWrittenToIdl() throws IOException {
Schema withDefault = Schema.createEnum("Status", null, "naming", asList("ACTIVE", "INACTIVE"), "ACTIVE");
StringWriter withDefaultWriter = new StringWriter();
IdlUtils.writeIdlProtocol(withDefaultWriter, withDefault);
assertTrue(withDefaultWriter.toString().contains("} = ACTIVE;"),
"Enum with default should serialize default value");
}
|
Hi @prygunov, sorry for taking so long to review this — thanks for the fix and for your patience. The change is correct and a clear improvement. The writer was asymmetric with A couple of things before we can merge:
Thanks again! |
…ation IdlUtils.writeSchema did not emit the enum default when serializing a schema to IDL, even though IdlReader parses it. As a result, an enum default was silently dropped when round-tripping a schema through IDL. Append "= <default>;" after the enum body when the schema has a default, matching the IDL grammar (RBrace defaultSymbol=enumDefault?), and add tests covering the with-default, without-default and write-then-parse round-trip cases.
The IdlUtils test class was never executed: Surefire only includes classes matching **/Test** (name starting with "Test"), but the class was named IdlUtilsTest, so its tests silently rotted since AVRO-3677. Rename IdlUtilsTest to TestIdlUtils so the suite runs, and fix the problems this uncovers: - byte[] values serialized to an empty string because the byte[] serializer discarded MAPPER.writeValueAsString(...) instead of writing to the generator; write the value to the generator. - The callToJson test helper had the same discard bug, so every *ToJson assertion previously compared against an empty string. - The happy-flow fixtures lived under org/apache/avro/util and were unreachable from the org.apache.avro.idl package; move them beside the test and regenerate them from the current writer output. - getMainSchema() now returns the record directly, so drop the obsolete union unwrapping in validateHappyFlowForSingleSchema. - Map/collection JSON expectations now include the ", " separator the MAPPER emits.
5d496c2 to
a052a75
Compare
|
Thanks @prygunov — the enum-default fix is correct. I pushed some changes directly to finish it up (force-pushed, so reset your local copy): 1. Squashed your 3 commits into one under your authorship with a proper 2. Added a maintainer commit fixing pre-existing breakage your PR surfaced. The
The |
What is the purpose of the change
This pull request fixes a bug in IdlUtils.writeSchema where enum schemas with a default value would silently lose that default when serialized to IDL format. As a result, round-tripping a schema through IDL would drop the enum default.
Verifying this change
Added enumDefaultIsWrittenToIdl test in IdlUtilsTest that:
Documentation