andygrove opened a new issue, #6335:
URL: https://github.com/apache/datafusion-comet/issues/6335
## What / Why
I audited how Comet handles timezones, starting from #2730. The model is
simple and mostly sound. Spark's `TimestampType` is a UTC instant, so nothing
is converted at the JVM/native boundary. Comet passes the raw microseconds in
both directions and labels them `Timestamp(Microsecond, "UTC")`.
`TimestampNTZType` is `Timestamp(Microsecond, None)`. The session timezone
never becomes part of a value. Each timezone-aware expression carries it, and
it's applied inside the native kernel, or the expression runs through the
codegen dispatcher with Spark's own `timeZoneId`.
The bugs cluster where that model breaks down:
- native expressions that emit a `TimestampType` value with some other label
- session timezone IDs that the native parser can't read
- timezone rules that come from a different database than the JVM's
Label drift is easy to miss. The scan and shuffle boundaries cast every
column back to its declared type, so a test that only projects the result
passes. It shows up when the result is compared, goes through a `CASE`, or
feeds another native expression.
All of the new bugs below reproduce on `main` at `764936187`, on Spark 3.5
and 4.1.
## Bugs
- [ ] #6328 `timestamp_seconds` returns a TIMESTAMP_NTZ-typed array, so
`hour`, casts and comparisons over it are wrong or fail in non-UTC sessions
(critical)
- [ ] #6330 `date_trunc` labels its output with the session timezone, so
comparing it with another timestamp fails in `Etc/UTC` sessions, the Linux
default (high)
- [ ] #6327 `CASE` and `COALESCE` over timestamps panic when the branches
have different Arrow timezones (high)
- [ ] #6329 session timezone IDs such as `GMT+8`, `Z` and `PST` make casts,
`hour` and `df.show()` fail (high)
- [ ] #5633 `timestamp_trunc` panics on DST-transition timestamps in a DST
timezone. Native only with `allowIncompatible` (high)
- [ ] #6331 native timezone rules come from chrono-tz's bundled tzdata
(2025b), not the JVM's (medium)
- [ ] #6332 the native CSV V2 scan ignores the session timezone (low,
testing-only config)
- [ ] #6333 `days` is evaluated in the session timezone, while `hours` and
Iceberg use UTC (low)
## The UTC fallbacks in #2730
I instrumented every `timeZoneId.getOrElse("UTC")` site. Then I ran the
datetime, cast, SQL-file, JSON, CSV, fuzz and expression suites. About 11,700
serde calls happened across 1,020 tests, and about 900 of them arrived without
a timezone. Every one of those was a cast that Spark doesn't consider
timezone-sensitive: numeric casts, Comet's own nullability-widening casts, and
the cast inside `IntegralDivide`. None was a timezone-aware expression, which
fits Spark refusing to resolve one without a timezone. So the fallback isn't a
correctness bug today. The helper proposed in #6329 would replace it. The
fallback can't simply be removed, though, because `array_with_timezone` asserts
a non-empty timezone even for casts that don't use one.
## Related
- #5456 date-to-timestamp casts overflow or panic for wide dates (#5457 is
open)
- #5010 datetime rebasing isn't supported (#5047 and #5048 are open).
Spark's legacy rebase is itself timezone-dependent.
- #4754 moving from chrono to jiff
- #4515 return type drift, where #6328 and #6330 are the timestamp cases
with consequences inside a stage
- #4180 has an item for honoring `spark.sql.session.timeZone` everywhere
Already documented: Python Arrow UDFs see timestamps labelled `UTC` rather
than the session timezone, and chrono-tz's DST rules end around 2100. Not in
the user guide yet: `spark.sql.parquet.int96TimestampConversion=true` disables
Comet for the session.
Fixed earlier, same class: #2720 (`SparkToColumnar` labelled timestamps with
the session timezone), #2649 via #4761 (the `date_trunc` schema mismatch, whose
fix introduced the label in #6330), and #5556 (the Python runner accepts
`Etc/UTC` for `UTC`).
## Test gaps
The SQL-file tests use `UTC`, `America/Los_Angeles`, `America/New_York`,
`Asia/Kolkata` and `+05:30`. None of them use `Etc/UTC`, or the offset and
short-ID forms from #6329. Most expression tests only project their result.
Adding `Etc/UTC` and `GMT+8` to the datetime files' `ConfigMatrix` would have
caught #6328, #6330, #6327 and #6329. So would tests that compare each native
timestamp-returning expression with another timestamp, or put it in a `CASE`.
--
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]
