stevomitric commented on code in PR #56677:
URL: https://github.com/apache/spark/pull/56677#discussion_r3459995402
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -158,6 +158,14 @@ object Cast extends QueryErrorsBase {
case (_: TimeType, _: TimeType) => true
case (_: TimeType, _: IntegralType) => true
+ // TIME(p) <-> TIMESTAMP_NTZ(q), q in [6, 9] (precision 6 is the micro
TimestampNTZType,
+ // [7, 9] is TimestampNTZNanosType). Restricted to the NTZ family on
purpose: TIMESTAMP_LTZ
+ // is not a valid counterpart for these casts.
+ case (_: TimeType, TimestampNTZType) => true
Review Comment:
No canANSIStoreAssign arm - the pair falls through the (_: DatetimeType, _:
DatetimeType) => true catch-all (:465), so TIME -> TIMESTAMP_NTZ becomes an
implicit ANSI store assignment: INSERT INTO t(ts_ntz) SELECT time_col silently
stamps CURRENT_DATE — the same injection the new UNION/CASE tests reject. Add
four => false arms before the catch-all + a store-assign test?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -1932,6 +1995,10 @@ case class Cast(
val zid = zoneIdValue(ctx)
(c, evPrim, evNull) =>
code"$evPrim = $dateTimeUtilsCls.convertTz($c.epochMicros,
java.time.ZoneOffset.UTC, $zid);"
+ case _: TimeType =>
+ val zid = zoneIdValue(ctx)
+ (c, evPrim, evNull) =>
+ code"$evPrim =
$dateTimeUtilsCls.makeTimestampNTZ($dateTimeUtilsCls.currentDate($zid), $c);"
Review Comment:
Nit: per-row currentDate read; fine since it's only the eval fallback
(ComputeCurrentTime rewrites it away), just not value-stable across midnight.
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/planner/SparkConnectProtoSuite.scala:
##########
@@ -986,6 +986,23 @@ class SparkConnectProtoSuite extends PlanTest with
SparkConnectPlanTest {
sparkTestRelation.select(col("id").cast("string")))
}
+ test("SPARK-57618: cast TIMESTAMP_NTZ to TIME") {
Review Comment:
Nit: only covers TIMESTAMP_NTZ -> TIME; the TIME -> TIMESTAMP_NTZ direction
is untested over Connect (TIME isn't in the DataType proto converter yet, as
the comment notes).
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/finishAnalysis.scala:
##########
@@ -119,8 +119,15 @@ object ComputeCurrentTime extends Rule[LogicalPlan] {
val currentDates = collection.mutable.HashMap.empty[ZoneId, Literal]
val localTimestamps = collection.mutable.HashMap.empty[ZoneId, Literal]
+ // The CAST bit is included so this rule can find TIME -> TIMESTAMP_NTZ
casts (which depend on
+ // CURRENT_DATE) and stabilize them below. CAST is a broad pattern, so
this widens the rule's
+ // traversal to most plans; the precise `Cast.isTimeToTimestampNTZ` guard
keeps the rewrite
+ // scoped. We intentionally do not tag these casts with CURRENT_LIKE
instead: inline-table
+ // validation treats CURRENT_LIKE as safe to defer, so tagging would let
unrelated non-foldable
+ // NTZ-target casts (e.g. CAST(rand() AS TIMESTAMP_NTZ)) bypass that
validation (see SPARK-57618
+ // and ResolveInlineTablesSuite).
def transformCondition(treePatternbits: TreePatternBits): Boolean = {
- treePatternbits.containsPattern(CURRENT_LIKE)
+ treePatternbits.containsPattern(CURRENT_LIKE) ||
treePatternbits.containsPattern(CAST)
Review Comment:
Pruning on CAST makes this rule traverse nearly every plan with a cast.
Worth a dedicated narrow pattern (TIME_TO_TIMESTAMP_NTZ_CAST tagged on Cast) to
keep traversal selective.
--
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]
