Skip to content

docs: update the timezone page for the fixes from the timezone EPIC - #6686

Merged
andygrove merged 1 commit into
apache:mainfrom
andygrove:docs/timezones-refresh
Oct 5, 2026
Merged

andygrove merged 1 commit into
apache:mainfrom
andygrove:docs/timezones-refresh

Conversation

@andygrove

Copy link
Copy Markdown
Member

Which issue does this PR close?

No issue closes. Part of #6335.

Rationale for this change

#6337 added timezones.md before the fixes from the same audit landed. Four of them changed behavior the page describes as current: #6347 (CASE and COALESCE no longer panic on a timestamp branch with another label), #6351 (session timezone IDs are normalized before they reach native code), #6353 (a warning when the native and JVM timezone databases differ), and #6349 (the native CSV scan falls back for timestamps outside UTC). The page still says the CASE casts panic, describes a timeZoneId.getOrElse("UTC") fallback that no longer exists on main, and says native code cannot parse GMT+8, Z or PST.

What changes are included in this PR?

  • "The invariant": CASE, COALESCE and IF now relabel a mismatched timestamp branch (coerce_branch in case_when.rs), so they move from the list of things that depend on the label to the places where a wrong label hides.
  • "How the session timezone reaches native code": serdes go through CometTimeZone.nativeId, an expression with no timezone gets "UTC", and native code reports an empty timezone as an error (require_timezone) instead of asserting.
  • "Parsing timezone IDs": which forms nativeId rewrites, and what happens to an offset with seconds such as +05:45:30.
  • "Timezone rules": the warning NativeBase logs when the tzdata versions differ, with a pointer to the user guide section.
  • "Scans": the native CSV scan's fallback.
  • "The codegen dispatcher" and "Guidelines": a timezone native code cannot express goes through the dispatcher, and timezones reach native code only through nativeId.
  • "Testing timezone-sensitive code": session_timezone_ids.sql, avoiding dates that recent tzdata releases changed, and checking that an expression ran natively rather than in the dispatcher, since a dispatched expression takes its whole subtree with it. It drops "put it in a CASE", which no longer exposes a wrong label.

#6340, which points the PR review skills at this page, is being rebased to match.

How are these changes tested?

This is a docs-only change. prettier --check passes. A local Sphinx build gives the same warnings with and without the change. I checked each statement against the code on main: CometTimeZone.scala, case_when.rs, utils.rs, NativeBase.java, CometScanRule.scala and CometScalaUDF.scala.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

  • Prior state and problem: The timezone contributor guide described behavior superseded by existing fixes.
  • Design approach: Updates the documentation to match current timezone normalization, conditional-branch coercion, database-version warnings, and CSV fallback behavior.
  • Correctness / compatibility analysis: The revised claims match the referenced Comet implementation. Checked Spark timezone resolution and CSV timestamp parsing against sources for supported versions 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0.
  • Key design decisions: Retains the timestamp-label invariant and points contributors to existing helpers. Introduces no runtime overhead or additional abstractions.
  • Implementation sketch: Changes only docs/source/contributor-guide/timezones.md, including guidance for verifying native execution and detecting dispatched subtrees.
  • Behavioral changes worth calling out: Documents existing fixes without changing execution. Compared with latest release branch branch-1.1, which does not contain this page. This PR itself introduces no runtime behavior change.
  • Suggested improvements: None at the P1/P2 threshold. No introduced P1/P2 issues found within this review.

Reviewed the entire diff from ba08acd815d5fd15f75ce9f314715778b9161cf1 to 2e8c9d9e74685bc2d19268406814ba58d5c32fb5. The PR is not a draft. Snapshot and live discussion checks found no existing reviews, comments, or review threads.

Routed skills: review-comet-pr. No sibling area skill applies to this documentation-only diff.

Exact-head CI: 7 checks passed and 16 were skipped, with no failures. Preflight passed Markdown formatting and repository checks. JVM/native builds, Spark SQL, and Iceberg suites were skipped.

Validation: Source comparisons, git diff --check, and all four relative Markdown link targets passed. No runtime suites were run. Sphinx is unavailable locally, so the rendered documentation was not built. The project worktree remains unchanged.

@andygrove
andygrove added this pull request to the merge queue Oct 5, 2026
Merged via the queue into apache:main with commit fa25924 Oct 5, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants