Re: [PR] [SPARK-56152][SQL] Enable implicit cast from STRING to TIME type [spark]

["cloud-fan (via GitHub)" <gi...@apache.org>]

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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org