MaxGekk commented on code in PR #56807:
URL: https://github.com/apache/spark/pull/56807#discussion_r3480913201
##########
connector/protobuf/src/main/scala/org/apache/spark/sql/protobuf/ProtobufDeserializer.scala:
##########
@@ -235,6 +235,11 @@ private[sql] class ProtobufDeserializer(
case (LONG, LongType) =>
(updater, ordinal, value) => updater.setLong(ordinal,
value.asInstanceOf[Long])
+ case (LONG, _: TimeType) =>
+ // The int64 field stores nanoseconds-of-day, matching TimeType's
internal
+ // nanos-since-midnight representation, so copy the value directly.
+ (updater, ordinal, value) => updater.setLong(ordinal,
value.asInstanceOf[Long])
Review Comment:
You're right, and thanks for the precise reading. Confirming: today there is
no non-test code path that hands an explicit `TimeType` schema to
`ProtobufDeserializer`. `from_protobuf` always derives its output schema from
the descriptor via `SchemaConverters.toSqlType`, which yields `LongType` for an
`int64`, so the `(LONG, TimeType)` branch is only reachable by constructing the
deserializer directly - which only the round-trip test does.
I'd prefer to keep the branch, for two reasons: (1) it's the exact inverse
of the serializer's `(TimeType, LONG)` case, so the serde pair stays symmetric
and the round-trip test actually validates the value mapping end-to-end rather
than just the write side; (2) it avoids a silent
`CANNOT_CONVERT_PROTOBUF_TYPE_TO_SQL_TYPE` gap if/when an explicit-schema
deserialization path is introduced. It's a 3-line, side-effect-free case.
I'll fix the PR description wording so it doesn't oversell - replacing "the
int64 -> TimeType deserializer path is exercised when..." with something like:
"the `int64 -> TimeType` branch is currently reachable only by constructing a
`ProtobufDeserializer` with an explicit `TimeType` schema (exercised by the
round-trip test); it is the inverse of the serializer case and is kept for
symmetry."
--
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]
