MaxGekk commented on code in PR #56764:
URL: https://github.com/apache/spark/pull/56764#discussion_r3472808876
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/types/DataTypeWriteCompatibilitySuite.scala:
##########
@@ -320,6 +320,50 @@ abstract class DataTypeWriteCompatibilityBaseSuite extends
SparkFunSuite {
}
}
+ test("SPARK-57564: Check TIME type write compatibility") {
+ // Expectations are derived from the policy-bound `canCast` (canUpCast for
the strict suite,
+ // canANSIStoreAssign for the ANSI suite), so this single test stays
correct under both
+ // subclasses: strict allows only TIME -> identical TIME, while ANSI
additionally allows
+ // writes across datetime types and across TIME precisions.
+ def checkPair(write: DataType, read: DataType): Unit = {
+ if (canCast(write, read)) {
+ assertAllowed(write, read, "t",
+ s"Should allow writing $write to $read because cast is safe")
+ } else {
+ val errs = new mutable.ArrayBuffer[String]()
+ checkError(
+ exception = intercept[AnalysisException] (
+ DataTypeUtils.canWrite("", write, read, true,
analysis.caseSensitiveResolution,
+ "t", storeAssignmentPolicy, errMsg => errs += errMsg)
+ ),
+ condition = "INCOMPATIBLE_DATA_FOR_TABLE.CANNOT_SAFELY_CAST",
+ parameters = Map(
+ "tableName" -> "``",
+ "colName" -> "`t`",
+ "srcType" -> toSQLType(write),
+ "targetType" -> toSQLType(read)
+ )
+ )
+ }
+ }
+
+ val timeTypes = Seq(TimeType(0), TimeType(3), TimeType(6))
+ // TIME -> TIME across all precision combinations (both directions via
full cross product).
Review Comment:
Non-blocking / optional consolidation. The three same-precision pairs from
this cross product (`TIME(0)->TIME(0)`, `TIME(3)->TIME(3)`, `TIME(6)->TIME(6)`)
re-cover the identity case that the shared `"Check each type with itself"` test
(line 290) already owns for every member of `allNonNullTypes`. That collection
(line 287) just omits `TimeType`.
Consider adding `Seq(TimeType())` to the `allNonNullTypes` flatten so the
shared identity test picks up TIME too, letting this dedicated test focus on
the non-trivial cross-precision and cross-type pairs. As a bonus it adds
genuinely-new `NullType <-> TIME` coverage via the two NullType-compatibility
tests (lines 110 / 253), which also iterate `allNonNullTypes`.
I checked it wouldn't break those three generic tests: `TimeType` behaves
like the other atomics in the NullType source paths, and their expected
`targetType` is computed dynamically from the iterated type, so NullType->TIME
is rejected under Strict and allowed under ANSI as expected. Entirely optional
— the current coverage is correct as-is.
--
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]
