cloud-fan commented on code in PR #54950:
URL: https://github.com/apache/spark/pull/54950#discussion_r3240578825
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionSuite.scala:
##########
@@ -1806,7 +1815,7 @@ object TypeCoercionSuite {
val fractionalTypes: Seq[DataType] =
Seq(DoubleType, FloatType, DecimalType.SYSTEM_DEFAULT, DecimalType(10, 2))
val numericTypes: Seq[DataType] = integralTypes ++ fractionalTypes
- val datetimeTypes: Seq[DataType] = Seq(DateType, TimestampType,
TimestampNTZType)
+ val datetimeTypes: Seq[DataType] = Seq(DateType, TimestampType,
TimestampNTZType, TimeType())
Review Comment:
Adding `TimeType()` here extends the existing `DateType` / `TimestampType` /
`TimestampNTZType` tests to assert that those can be implicitly cast to
`TimeType` (and the new TimeType test asserts the reverse). At the
`implicitCast` level this succeeds via `(_: DatetimeType, d: DatetimeType) =>
d` (`TypeCoercion.scala:223`) and `canANSIStoreAssign`'s `(_: DatetimeType, _:
DatetimeType) => true` (`Cast.scala:393`). But `Cast.canCast` /
`Cast.canAnsiCast` have no rule for `DateType <-> TimeType`, `TimestampType <->
TimeType`, `TimestampNTZType <-> TimeType` -- and `findWiderDateTimeType`
explicitly excludes TimeType (`TypeCoercionHelper.scala:245-246`). The test
passes only because `shouldCast` checks `dataType`, not
`Cast.checkInputDataTypes`. The drift is preexisting, but folding `TimeType`
into the shared dataset suggests cross-castability that isn't actually
supported. Consider keeping the new TimeType test restricted to `castableTypes
= Seq(checkedType, StringType)` instead of pu
lling `TimeType` into the shared `datetimeTypes`.
##########
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)
++ datetimeTypes)
+ shouldCast(checkedType, AnyTimeType, AnyTimeType.defaultConcreteType)
Review Comment:
This is the only new assertion that mentions `AnyTimeType`, but it tests
`TimeType -> AnyTimeType` -- which already succeeded via
`expectedType.acceptsType(inType)` (line 173 of `AnsiTypeCoercion.scala`)
before this PR. To exercise the new `(_: StringType, ... | AnyTimeType)` branch
you added in `AnsiTypeCoercion.scala:195-196` (and the new case at
`TypeCoercion.scala:231`), please also add `shouldCast(checkedType,
AnyTimeType, AnyTimeType.defaultConcreteType)` to the `StringType` test in
`TypeCoercionSuite.scala` (around line 534) and `AnsiTypeCoercionSuite.scala`
(around line 90), mirroring the existing `shouldCast(checkedType,
AnyTimestampType, ...)`. As written, reverting the two production-code
additions doesn't fail any test in this PR.
##########
sql/core/src/test/resources/sql-tests/inputs/typeCoercion/native/implicitTypeCasts.sql:
##########
@@ -56,6 +56,10 @@ SELECT length('four') FROM t;
SELECT length(date('1996-09-10')) FROM t;
SELECT length(timestamp('1996-09-10 10:11:12.4')) FROM t;
+-- string to time
+SELECT '12:00:00' = TIME'12:00:00' FROM t;
+SELECT '12:00:01' > TIME'12:00:00' FROM t;
Review Comment:
These comparisons go through `StringPromotionTypeCoercion` /
`AnsiStringPromotionTypeCoercion` for binary comparisons (`(l: StringType, r:
AtomicType) if canPromoteAsInBinaryComparison(r)` and `(_: StringType, a:
AtomicType) => Some(a)`), not the `implicitCast` rule modified in this PR. Both
pass without the production-code changes. Consider also adding a call that
flows through `ImplicitTypeCasts` against an `AnyTimeType` input -- e.g.,
`SELECT time_trunc('HOUR', '12:34:56') FROM t` (`TimeTrunc` mixes in
`ImplicitCastInputTypes` with `inputTypes: Seq(StringType, AnyTimeType)`, so
the second argument's coercion exercises the new rule).
--
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]
