HyukjinKwon commented on code in PR #59394:
URL: https://github.com/apache/spark/pull/59394#discussion_r4268420127
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:
##########
@@ -429,6 +431,21 @@ object Cast extends QueryErrorsBase {
case (fromField, toField) =>
needsTimeZone(fromField.dataType, toField.dataType)
}
+ // `castToString` recurses into ARRAY/MAP/STRUCT and formats each LTZ leaf
with `zoneId`, while
+ // the complex-to-complex cases above only match a target of the same
shape.
+ case (from, _: StringType) => from.existsRecursively(needsTimeZoneToString)
+ case _ => false
+ }
+
+ /**
+ * Whether rendering a value of this type as a string reads the session time
zone. Only the LTZ
+ * families carry an absolute instant that has to be localized;
TIMESTAMP_NTZ, DATE and TIME
+ * render on a fixed grid. VARIANT may hold a timestamp at runtime, as in
`needsTimeZone`.
+ */
+ private def needsTimeZoneToString(dt: DataType): Boolean = dt match {
+ case TimestampType | _: TimestampLTZNanosType | VariantType => true
Review Comment:
**Nit (P3):** This also marks a nested `VARIANT` as zone-dependent, but the
nested path never reads the zone. Only a top-level VARIANT source goes through
`VariantGet.cast(v, to, castArgs)` and renders with `castArgs.zoneId`, which is
what the existing `(VariantType, _)` arm covers. A VARIANT under
ARRAY/MAP/STRUCT is rendered by `ToStringBase.castToString` instead.
`TypeApiOps(VariantType, ...)` is `None`, so it falls to the default
`o.toString` (codegen: `String.valueOf($c)`). That is `VariantVal.toString`,
i.e. `toJson(ZoneOffset.UTC)`.
So `CAST(array(CAST(ts AS VARIANT)) AS STRING)` returns the same string in
every session zone. With this change it now needs a zone to resolve, keeps the
zone in `canonicalized` (no cache reuse or `semanticEquals` across zones), and
is no longer `contextIndependentFoldable`. That breaks the `needsTimeZone`
contract ("exactly when its `castTo*` / `castTo*Code` path reads `zoneId`").
Results are still correct; the classification is just over-conservative.
Could we drop `VariantType` here and keep the top-level `(VariantType, _)`
arm? That would also mean rewording the "VARIANT may hold a timestamp at
runtime" sentence above and moving `ArrayType(VariantType)` in the new test to
the zone-independent list. `ARRAY<VARIANT> -> ARRAY<STRING>` would still be
zone-dependent through the top-level arm.
```suggestion
case TimestampType | _: TimestampLTZNanosType => true
```
**Verification:**
- **Behavior:** The `needsTimeZone` classification test asserts nested
VARIANT under ARRAY (and at least one of MAP or STRUCT) is zone-independent
when cast to STRING, while top-level VARIANT -> STRING and ARRAY<VARIANT> ->
ARRAY<STRING> remain zone-dependent.
- **Regression:** The existing nested-LTZ positives, NTZ/DATE/TIME
negatives, the resolution and semantic-equality checks, and the cross-time-zone
cache-reuse test still pass unchanged.
--
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]
