sachinnn99 commented on PR #38194: URL: https://github.com/apache/beam/pull/38194#issuecomment-4258740259
Thanks for the review @ahmedabu98! Two follow-ups: **Naming:** Agreed. Planning to rename the helper to `inferredLogicalTypeFor` so additional LogicalType inferences (Decimal/URI/etc.) extend that one helper rather than spawning per-type methods. **CI — Avro regression:** The "failure" status on CI hides a real issue I want to flag before you merge: `AvroSchemaTest.testSpecificRecordToRow` and `testRowToSpecificRecord` fail with `ClassCastException`/`AssertionError`. Root cause is that the new `javaTimeLogicalTypeFor(...)` check in `TypeConversion.convert()` is a concrete-class whitelist placed ahead of `convertDefault`, while every other branch in that method is structural (`isSubtypeOf`, `isArray`, `isPrimitive`, `isEnum`). Avro's `convertDefault` overrides for `java.time.Instant` / `java.time.LocalDate` (which bridge to Joda `Instant` for Beam's `DATETIME` Row representation) are now dead code — they never run. Two ways to fix: 1. Override `convertJavaTimeLogicalType` in the three Avro subclasses to apply the existing Joda conversion. Follows the `convertDateTime` precedent. 2. Fold the JSR-310/UUID handling into `convertDefault()` of the three base subclasses, drop the new abstract method + dispatch branch. Top-level dispatch stays structural; Avro's existing `convertDefault` override handles this naturally (with `super.convertDefault` fallback for `LocalTime`/`LocalDateTime`/`UUID` which Avro doesn't special-case). Zero changes to `AvroUtils.java`. I'm leaning toward (2) since `convertDefault` is already where Avro holds its concrete-class checks, but happy to go with (1) if you prefer sticking closer to the `convertDateTime` precedent. Which direction do you prefer? -- 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]
