andygrove opened a new pull request, #6351:
URL: https://github.com/apache/datafusion-comet/pull/6351
## Which issue does this PR close?
Closes #6329.
## Rationale for this change
Comet passed the session timezone to native code as the raw string stamped
on each expression. Native code parses that string with arrow's `Tz::from_str`,
which only accepts:
- IANA zone names
- offsets written as `+HH`, `+HHMM` or `+HH:MM`
Spark resolves the ID with `ZoneId.of(id, ZoneId.SHORT_IDS)`, which also
accepts:
- `Z`
- offsets such as `+8` and `+08:00:00`
- prefixed offsets such as `GMT+8` and `UTC+08:00`
- short IDs such as `PST` and `IST`
The `spark.sql.session.timeZone` documentation lists `Z` and `(+|-)HH:mm:ss`
explicitly. With any of these IDs, most timestamp expressions failed at
execution time with `Parser error: Invalid timezone "GMT+8"`. That covered
`CAST(ts AS STRING/DATE)`, `hour`, `year`, string- and date-to-timestamp casts,
`unix_timestamp` of a date, casts between `TIMESTAMP` and `TIMESTAMP_NTZ`, and
`df.show()`. `GMT+8` in particular is a common production setting.
## What changes are included in this PR?
- A new `CometTimeZone.nativeId` normalizes the stamped timezone into an ID
native code parses. It uses Spark's own `DateTimeUtils.getZoneId`, then
`ZoneId.normalized()`:
- fixed offsets become `+HH:MM`
- zero offsets become `UTC`
- short IDs become their region
- an expression with no timezone gets `UTC`, since Spark only leaves it
unset on casts that don't use one (the measurement is in #6335)
- An offset with seconds, such as `+05:45:30`, can't be written for native
code. `nativeId` returns `None` for it, and the serde reports the expression as
unsupported. The cast then goes through the codegen dispatcher, and expressions
without a dispatcher path fall back.
- Every serde that sends a timezone to native code now goes through the
helper. That covers `Cast`, `hour`, `minute`, `second`, `unix_timestamp`,
`date_trunc`, `from_unixtime`, `to_json`, `from_json`, `to_csv`,
`ToPrettyString` (both shims) and the native Parquet scan's session timezone.
These were the `getOrElse("UTC")` sites that #2730 asked about.
- `date_trunc` and `date_format` decide "is this session UTC" through the
same helper. `GMT`, `Z` and `+00:00` sessions now take their native UTC paths
as well.
- `CometDays` is left alone, because #6348 moves it to UTC.
## How are these changes tested?
- **New tests:**
- A unit test in `CometTemporalExpressionSuite` for the normalization.
- `session_timezone_ids.sql`, which runs casts, field extraction,
`unix_timestamp`, `TIMESTAMP_NTZ` casts, `date_trunc` and `date_format` under
`GMT+8`, `UTC+08:00`, `+8`, `-08`, `+08:00:00`, `Z`, `PST` and `IST`.
- `session_timezone_ids_unsupported.sql`, which covers `+05:45:30`.
- **Before the change:** with `main`'s serdes, `session_timezone_ids.sql`
fails for every timezone except `-08`, which arrow already parses. The
unsupported file fails too.
- **With the change on Spark 4.1:** all 200 tests in these suites pass, with
the `spotless` and `scalastyle` checks included:
- `CometTemporalExpressionSuite`
- every `expressions/datetime/` SQL file test
- `CometJsonExpressionSuite`
- `CometCsvExpressionSuite`
- **Casts:** all 185 `CometNativeCastSuite` tests pass on Spark 4.1.
- **Spark 3.5:** the new tests pass on Spark 3.5 as well, which also
compiles the 3.5 `ToPrettyString` shim.
--
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]
