MaxGekk commented on code in PR #56439:
URL: https://github.com/apache/spark/pull/56439#discussion_r3394244343
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SerializerBuildHelper.scala:
##########
@@ -384,8 +384,6 @@ object SerializerBuildHelper {
case TimestampEncoder(false) => createSerializerForSqlTimestamp(input)
case InstantEncoder(false) => createSerializerForJavaInstant(input)
case LocalDateTimeEncoder => createSerializerForLocalDateTime(input)
- case LocalTimeEncoder if !SQLConf.get.isTimeTypeEnabled =>
- throw
org.apache.spark.sql.errors.QueryCompilationErrors.unsupportedTimeTypeError()
case LocalTimeEncoder => createSerializerForLocalTime(input)
Review Comment:
This arm is now unreachable: `createSerializer` routes `LocalTimeEncoder`
through `TypeOps(enc.dataType)` before this match, and
`TimeTypeOps.createSerializer` always returns `Some` or throws, so the
`getOrElse` never falls through to here. The
`OptionEncoder`/`TransformingEncoder` arms recurse back through the dispatch,
so they can't reach it either. The same holds for the twin arm in
`DeserializerBuildHelper`, and
`createSerializerForLocalTime`/`createDeserializerForLocalTime` have no other
callers, so they die with the arms. Since the PR's goal is collapsing the two
paths into one, I'd delete all four here.
The framework now also always intercepts the wider legacy TIME fallbacks —
`Row.toJsonDefault`'s TIME arm (whose regression test this PR removes),
`HiveResult.toHiveString`, and the TIME/LocalTime arms in
`DataTypeProtoConverter`/`LiteralValueProtoConverter`. Is a follow-up cleanup
planned for those?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/types/ops/TimeTypeOps.scala:
##########
@@ -92,7 +94,18 @@ case class TimeTypeOps(override val t: TimeType) extends
TimeTypeApiOps(t) with
// ==================== Optional Operations ====================
+ // Preserve the pre-framework gate: building a TIME serializer/deserializer
requires the TIME
+ // type to be enabled (mirrors the legacy `LocalTimeEncoder if
!isTimeTypeEnabled` arms in
+ // Serializer/DeserializerBuildHelper that this dispatch replaces). Throw
rather than return
+ // None so the dispatch surfaces UNSUPPORTED_TIME_TYPE instead of falling
through to a legacy arm.
Review Comment:
The gated arms this comment cites were deleted by this PR, so a post-merge
reader can't find them. Worth rewording to the current invariant:
```suggestion
// Building a TIME serializer/deserializer requires the TIME type to be
enabled. The framework
// dispatch at the head of Serializer/DeserializerBuildHelper routes
LocalTimeEncoder through
// these methods, so this is the single gate on the encoder path. Throw
rather than return None
// so the dispatch surfaces UNSUPPORTED_TIME_TYPE instead of falling back
to the default arms.
```
--
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]
