iemejia commented on PR #3822: URL: https://github.com/apache/avro/pull/3822#issuecomment-5259387976
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 `IdlReader` (which already parses enum defaults via `exitEnumDefault`), so any enum with a default was silently dropped on round-trip. The fix matches both the IDL grammar (`RBrace defaultSymbol=enumDefault?` → `} = SYMBOL;`) and the existing fixtures (`status_schema.avdl`, `simple.avdl`). I applied it locally and confirmed a full write → parse round-trip now preserves `getEnumDefault()`, with no change for enums that have no default. A couple of things before we can merge: 1. **JIRA ticket + title.** Per our contribution guidelines, PRs need an associated JIRA issue and the title should follow `AVRO-XXXX: [java] ...`. Could you please create a ticket at https://issues.apache.org/jira/projects/AVRO/issues/ and update the PR title accordingly (e.g. `AVRO-XXXX: [java] Add support for enum default values in IDL serialization`)? 2. **Strengthen the test.** The current test only covers the positive case, though the PR description also mentions the "no default" case. Could you add: - a negative assertion that an enum without a default emits no `=`, and - a real round-trip assertion that proves the reported bug is fixed — write to IDL, parse it back, and check the default survives: ```java IdlReader reader = new IdlReader(); Schema roundTripped; try (InputStream in = new ByteArrayInputStream(withDefaultWriter.toString().getBytes(StandardCharsets.UTF_8))) { roundTripped = reader.parse(in).getNamedSchemas().get("naming.Status"); } assertEquals("ACTIVE", roundTripped.getEnumDefault()); ``` Thanks again! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
