cloud-fan commented on code in PR #54950:
URL: https://github.com/apache/spark/pull/54950#discussion_r3242241958
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionSuite.scala:
##########
@@ -217,6 +217,15 @@ abstract class TypeCoercionSuiteBase extends AnalysisTest {
shouldNotCast(checkedType, IntegralType)
}
+ test("SPARK-56152: implicit type cast - TimeType") {
+ val checkedType = TimeType()
+ checkTypeCasting(checkedType, castableTypes = Seq(checkedType, StringType))
Review Comment:
The new `SPARK-56152: implicit type cast - TimeType` test fails in both
`TypeCoercionSuite` and `AnsiTypeCoercionSuite`. Running locally:
```
- SPARK-56152: implicit type cast - TimeType *** FAILED ***
Some(cast(00:00:00 as date)) was not empty
Should not be able to cast TimeType(6) to DateType, but got
Some(cast(00:00:00 as date))
```
`checkTypeCasting` calls `shouldNotCast(TimeType, X)` for every `X` in
`allTypes \ castableTypes`. With `castableTypes = Seq(checkedType,
StringType)`, that set now includes `DateType`, `TimestampType`,
`TimestampNTZType` (since `datetimeTypes` was reverted to exclude `TimeType` in
the same commit). But `implicitCast(TimeType, DateType)` returns
`Some(DateType)`:
- `TypeCoercion.scala:223` has `case (_: DatetimeType, d: DatetimeType) =>
d` -- `TimeType` is a `DatetimeType`.
- On the ANSI side, `Cast.canANSIStoreAssign(TimeType, DateType) = true` via
`Cast.scala:393` `(_: DatetimeType, _: DatetimeType) => true`.
This is the drift my prior comment flagged. My suggested restriction was
tighter than what `implicitCast` actually does today -- apologies for the
misleading advice. The fix that matches reality while preserving the original
concern (no implied self-castability through the shared dataset) is to keep the
shared set, now that `datetimeTypes` itself excludes `TimeType`:
```suggestion
checkTypeCasting(checkedType, castableTypes = Seq(checkedType,
StringType) ++ datetimeTypes)
```
Properly closing the underlying drift (excluding `TimeType` from the generic
`DatetimeType -> DatetimeType` branch in `implicitCast` and from
`canANSIStoreAssign`) is a bigger change and out of scope here.
--
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]
