MaxGekk commented on PR #56266: URL: https://github.com/apache/spark/pull/56266#issuecomment-4612938176
Thanks for the review @davidm-db @stevomitric. Addressing the two review-summary comments: **@davidm-db (description wording).** You're right - only `getBoxedJavaClass` is newly added to the `TypeOps` base in this PR; `createSerializer` / `createDeserializer` already exist on the base trait and are used by `TimeTypeOps`, and here they're just overridden on the nanos ops. I've reworded the "What changes were proposed" section so it no longer implies base-trait additions that aren't in the diff. **@stevomitric (wrapper encoders bypassing the framework serde dispatch).** Good catch. Since `OptionEncoder` / `TransformingEncoder` proxy `dataType` to the wrapped encoder, `OptionEncoder(LocalDateTimeNanosEncoder(p)).dataType == TimestampNTZNanosType(p)`, so the framework `createSerializer` / `createDeserializer` dispatch (which keys off `enc.dataType`) fired on the wrapper and skipped `UnwrapOption` / `WrapOption` (and the codec for `TransformingEncoder`). Fixed in 02db736: `SerializerBuildHelper.createSerializer` / `DeserializerBuildHelper.createDeserializer` now route `OptionEncoder` / `TransformingEncoder` through the default path first, so the framework leaf dispatch only ever sees unwrapped leaf encoders. Added a roundtrip test for Option-wrapped nanos encoders (`Some` / `None`, NTZ and LTZ). For context: this dispatch ordering predates this PR (the helpers have routed through `TypeOps(enc.dataType)` since the serde hooks landed, and `TimeTypeOps` already overrides them), so the same fix also covers `Option[LocalTime]` etc. The nanos leaf encoders aren't reachable via Scala reflection today (`Option[LocalDateTime]` resolves to the micros `LocalDateTimeEncoder`), so the gap was latent - but the ordering was fragile, so better to make it robust. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
