[SPARK-60179][SQL] Treat a cast to STRING as time-zone dependent when the source type contains a nested TIMESTAMP_LTZ by hhr293 · Pull Request #59394 · apache/spark

hhr293 · 2026-10-12T08:17:43Z

What changes were proposed in this pull request?

Cast.needsTimeZone incorrectly treats complex-to-STRING casts containing nested LTZ timestamps as time-zone independent, even though castToString uses the session time zone.

This PR fixes the classification by recursively checking the source type, including Python UDTs. TIMESTAMP_NTZ, DATE and TIME remain time-zone independent.

Why are the changes needed?

The incorrect classification can cause unresolved time-zone access, incorrect expression equivalence, and wrong results when cached queries are reused across session time zones.

Does this PR introduce any user-facing change?

Yes. Complex-to-STRING casts now correctly preserve time-zone semantics, preventing incorrect cross-time-zone cache reuse.

How was this patch tested?

Added tests for nested types, Python UDTs, expression resolution, semantic equality, and cross-time-zone cache reuse.
Mutation testing confirmed that the affected cache tests fail without the fix, while the scalar TIMESTAMP control still passes.
All 276 Cast tests and 107 CachedTableSuite tests pass.

Was this patch authored or co-authored using generative AI tooling?

Yes. Co-authored-by: Claude Code (Claude Opus 5)

… the source holds a timestamp `Cast.needsTimeZone` recursed only into casts whose target had the same shape as the source, so collapsing a complex type to a single STRING was reported as time-zone independent even though `castToString` renders every nested LTZ timestamp with the session `zoneId`. Such a cast was therefore resolved without a `timeZoneId` (and then failed in `zoneId`), reported `contextIndependentFoldable`, and had its zone dropped by `canonicalized`, which let a plan cached under one session time zone serve a query running under another. Co-authored-by: Lipeng Zhu <[email protected]> Co-authored-by: Wangyang Guo <[email protected]>

HyukjinKwon

Review summary

The fix is placed at the right owner. Every consumer of the classification (Cast.resolved, canonicalized, Cast/UpCast.contextIndependentFoldable, and the zone-inserting call sites) reads Cast.needsTimeZone, and the new arm mirrors ToStringBase's recursion and leaf formatters for LTZ timestamps and Python UDTs. Both analyzers assign a zone to every time-zone-aware expression, so the stricter resolved cannot leave analyzed plans unresolved. The new tests fail without the fix at each layer: the predicate, resolution and semantic equality, and end-to-end cache reuse.

One non-blocking issue: the new leaf predicate also flags nested VARIANT, whose string rendering is fixed at UTC. Those casts therefore lose cross-zone cache reuse and V2 folding even though their output does not change, and the helper comment and one test row describe behavior the renderer does not have.

Findings

1 total: 0 P0, 0 P1, 0 P2, 1 P3.

Nit (P3)

Nested VARIANT is classified as zone-dependent for STRING casts, but its rendering is fixed at UTC — sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:446 — see inline.

PR description suggestions

The PR title is cut off with an ellipsis ("... time-zone dependent when…"). Since it becomes the commit subject, consider completing it, e.g. "[SPARK-60179][SQL] Treat a cast to STRING as time-zone dependent when the source type contains a nested TIMESTAMP_LTZ".

Generated by Omnigent on Databricks.

HyukjinKwon · 2026-10-14T08:23:35Z

+   * 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


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.

Suggested change

case TimestampType | _: TimestampLTZNanosType | VariantType => true

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 -> ARRAY 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.

Thanks for the detailed review! You're right — nested VARIANT-to-STRING rendering uses UTC and does not depend on the session time zone.

I'll remove VariantType from the helper, update the comment, and adjust the tests to cover both nested and element-wise VARIANT casts.

A nested VARIANT has no `castToString` case of its own, so it renders through `VariantVal.toString`, which pins the zone to UTC. Only a top-level VARIANT source goes through `VariantGet.cast` with the session zone, which the existing `(VariantType, _)` arm already covers. Co-authored-by: Lipeng Zhu <[email protected]> Co-authored-by: Wangyang Guo <[email protected]>

hhr293 · 2026-10-16T01:50:05Z

@HyukjinKwon Hi, could you please take a look at this PR when you have a chance? It fixes incorrect time-zone handling for complex-to-STRING casts. Your feedback would be greatly appreciated. Thanks!

hhr293 · 2026-10-22T09:21:16Z

Hi, @sunchao @cloud-fan could you please take a look at this PR when you have a chance? Thanks a lot.

cloud-fan

Review summary

The fix is at the right owner. Cast.needsTimeZone is the single zone classifier, and the new (from, _: StringType) arm applies the LTZ leaf rule through existsRecursively. This matches how ToStringBase renders nested values: TIMESTAMP and TIMESTAMP_LTZ(p) leaves read zoneId, while TIMESTAMP_NTZ, DATE, TIME, Scala UDTs, and a VARIANT nested under ARRAY/MAP/STRUCT do not. Every consumer of the classification picks up the change with no further edits:

resolved
canonicalized, and through it cache matching and semanticEquals
contextIndependentFoldable
the call sites that attach a time zone

Both analyzers attach the session zone to every Cast, so the stricter resolved cannot leave analyzed plans unresolved. The new tests fail without the fix at the classification, expression, and cache-reuse layers.

The earlier nested-VARIANT comment is addressed at head:

The helper no longer matches VariantType.
Its comment explains that a nested VARIANT renders at UTC.
The tests list nested VARIANT as zone-independent, while top-level VARIANT to STRING and ARRAY to ARRAY stay zone-dependent.

One nit remains: the new helper's scaladoc says it answers for a whole type, but the helper is a per-node check that returns false for ARRAY/MAP/STRUCT.

Prior-review input still awaiting a decision:

The earlier review suggested completing the truncated PR title. The title still ends mid-sentence. (prior review)

Findings

1 total: 0 P0, 0 P1, 0 P2, 1 P3.

Nit (P3)

needsTimeZoneToString scaladoc describes a whole-type check, but the helper is per-node — sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala:441 — see inline.

PR description suggestions

The PR title is still cut off mid-sentence ("... time-zone dependent when..."). Since it becomes the commit subject, consider completing it, e.g. "[SPARK-60179][SQL] Treat a cast to STRING as time-zone dependent when the source type contains a nested TIMESTAMP_LTZ".

cloud-fan · 2026-10-22T12:17:53Z

+  }
+
+  /**
+   * Whether rendering a value of this type as a string reads the session time zone. Only the LTZ


Nit (P3): This reads as a whole-type contract, but the helper is a per-node check. needsTimeZoneToString(ArrayType(TimestampType)) returns false, because ARRAY/MAP/STRUCT nodes fall through to case _ => false, yet rendering an ARRAY reads zoneId. The whole-type answer comes only from the caller's from.existsRecursively(needsTimeZoneToString). The current call site is correct, but a future direct call on a container type would misclassify the cast. Could the first sentence say it is a per-node check? For example:

* Whether this type node, taken as a leaf, reads the session time zone when rendered as a * string. ARRAY/MAP/STRUCT nodes return false; apply it through `existsRecursively` to cover * nested types.

The rest of the comment, which explains why TIMESTAMP_NTZ, DATE, TIME, and a nested VARIANT are zone-independent, can stay as is.

Verification:

Inspection: The reworded comment says the helper classifies one type node (TIMESTAMP and TIMESTAMP_LTZ(p) leaves, and Python UDTs through their sqlType), that container traversal comes from existsRecursively, and it still explains why TIMESTAMP_NTZ, DATE, TIME, and a nested VARIANT are zone-independent.

cloud-fan · 2026-10-22T12:17:56Z

Thanks for the ping, @hhr293. I've taken a look; please see my review.

sunchao

LGTM at 64a2975cb05d40f6d28125e5732047f1c56af4f9.

Completed five independent review passes covering interpreted/code-generated rendering, analyzer and optimizer consumers, canonicalization/cache matching, adversarial nested types and UDTs, and regression-test coverage. I found no new correctness or regression issues. The recursive classifier matches the rendering paths, including LTZ nanos, Python UDTs, and the distinction between nested and top-level VARIANT. The existing documentation nit is non-blocking.

Validation: inspected the exact-commit CI run and its XML reports. CastWithAnsiOnSuite (137), CastWithAnsiOffSuite (139), TryCastSuite (141), and CachedTableSuite (107) all passed with zero failures or skips; the new SPARK-60179 cases are present and passed. Local git diff --check passed. I did not rerun the JVM suites locally.

Comment-only change: no code or test is modified. Reword the scaladoc of needsTimeZoneToString to say it is a per-node check, with container traversal done by the caller via existsRecursively. Co-Authored-By: Claude Sonnet 5.5 <[email protected]>

hhr293 · 2026-10-23T02:35:15Z

Thanks @cloud-fan and @sunchao for the reviews and approvals! I've addressed the documentation nit and updated the PR title.

… the source type contains a nested TIMESTAMP_LTZ ### What changes were proposed in this pull request? `Cast.needsTimeZone` incorrectly treats complex-to-STRING casts containing nested LTZ timestamps as time-zone independent, even though `castToString` uses the session time zone. This PR fixes the classification by recursively checking the source type, including Python UDTs. TIMESTAMP_NTZ, DATE and TIME remain time-zone independent. ### Why are the changes needed? The incorrect classification can cause unresolved time-zone access, incorrect expression equivalence, and wrong results when cached queries are reused across session time zones. ### Does this PR introduce any user-facing change? Yes. Complex-to-STRING casts now correctly preserve time-zone semantics, preventing incorrect cross-time-zone cache reuse. ### How was this patch tested? - Added tests for nested types, Python UDTs, expression resolution, semantic equality, and cross-time-zone cache reuse. - Mutation testing confirmed that the affected cache tests fail without the fix, while the scalar TIMESTAMP control still passes. - All 276 Cast tests and 107 CachedTableSuite tests pass. ### Was this patch authored or co-authored using generative AI tooling? Yes. Co-authored-by: Claude Code (Claude Opus 5) Closes #59394 from hhr293/SPARK-60179. Authored-by: hu hengrui <[email protected]> Signed-off-by: Chao Sun <[email protected]> (cherry picked from commit 11e2e6c) Signed-off-by: Chao Sun <[email protected]>

sunchao · 2026-10-23T15:24:36Z

Merge Summary:

merged into master 11e2e6c
merged into branch-4.x f7c3917

Posted by merge_spark_pr.py

HyukjinKwon reviewed Oct 14, 2026

View reviewed changes

hhr293 mentioned this pull request Oct 14, 2026

[SPARK-41416][SQL] Rewrite existence-only inequality self-joins as MIN/MAX #58424

Open

hhr293 requested a review from HyukjinKwon October 15, 2026 02:14

cloud-fan approved these changes Oct 22, 2026

View reviewed changes

sunchao approved these changes Oct 22, 2026

View reviewed changes

hhr293 changed the title ~~[SPARK-60179][SQL] Treat a cast to STRING as time-zone dependent when…~~ [SPARK-60179][SQL] Treat a cast to STRING as time-zone dependent when the source type contains a nested TIMESTAMP_LTZ Oct 23, 2026

sunchao closed this in 11e2e6c Oct 23, 2026

Navigation Menu

Repository navigation

[SPARK-60179][SQL] Treat a cast to STRING as time-zone dependent when the source type contains a nested TIMESTAMP_LTZ - #59394

[SPARK-60179][SQL] Treat a cast to STRING as time-zone dependent when the source type contains a nested TIMESTAMP_LTZ#59394
hhr293 wants to merge 3 commits into
apache:masterfrom
hhr293:SPARK-60179

hhr293 commented Oct 12, 2026 •

edited

Loading

Uh oh!

HyukjinKwon left a comment

Uh oh!

HyukjinKwon Oct 14, 2026

Uh oh!

hhr293 Oct 14, 2026

Uh oh!

hhr293 commented Oct 16, 2026

Uh oh!

hhr293 commented Oct 22, 2026

Uh oh!

cloud-fan left a comment

Uh oh!

cloud-fan Oct 22, 2026

Uh oh!

cloud-fan commented Oct 22, 2026

Uh oh!

sunchao left a comment

Uh oh!

hhr293 commented Oct 23, 2026

Uh oh!

sunchao commented Oct 23, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	case TimestampType \| _: TimestampLTZNanosType \| VariantType => true
	case TimestampType \| _: TimestampLTZNanosType => true

Conversation

hhr293 commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Review summary

Findings

Nit (P3)

PR description suggestions

Uh oh!

HyukjinKwon Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

hhr293 Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

hhr293 commented Oct 16, 2026

Uh oh!

hhr293 commented Oct 22, 2026

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Review summary

Findings

Nit (P3)

PR description suggestions

Uh oh!

cloud-fan Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Oct 22, 2026

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

hhr293 commented Oct 23, 2026

Uh oh!

sunchao commented Oct 23, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hhr293 commented Oct 12, 2026 •

edited

Loading