andygrove opened a new pull request, #6341:
URL: https://github.com/apache/datafusion-comet/pull/6341
## Which issue does this PR close?
Closes #6328.
## Rationale for this change
The native `seconds_to_timestamp` function, which Comet uses for
`timestamp_seconds`, declared and returned `Timestamp(Microsecond, None)`. That
is the Arrow type Comet uses for `TIMESTAMP_NTZ`. Spark's result is
`TimestampType`, which Comet represents as `Timestamp(Microsecond, "UTC")`
everywhere else.
Projecting the result directly looked fine. Expressions that consumed it did
not:
- In a non-UTC session, `hour`, `CAST(... AS STRING)`, `CAST(... AS DATE)`
and `CAST(... AS TIMESTAMP_NTZ)` treated the result as wall-clock time and
silently ignored the session timezone.
- Comparing the result with another timestamp failed even in UTC.
- Mixing it with another timestamp in `CASE` or `coalesce` panicked, also in
UTC.
The bug has been there since `timestamp_seconds` went native in #3146, so
1.0.0 and branch-1.1 have it too.
## What changes are included in this PR?
- `seconds_to_timestamp` now returns `Timestamp(Microsecond, "UTC")` from
`return_type` and stamps that label on every array and scalar it produces. The
values are unchanged. It uses `with_timezone("UTC")` rather than arrow's
`with_timezone_utc()`, because the latter labels the array `+00:00`, which is a
different Arrow type.
- New Rust unit tests check the declared type and the output label for each
input type, for both arrays and scalars.
- A new SQL file test, `timestamp_seconds_timezone.sql`, runs in `UTC`,
`America/Los_Angeles` and `Asia/Kolkata`. It uses the result rather than only
projecting it: `hour` and `minute`, casts to string, date and `TIMESTAMP_NTZ`,
comparisons with a timestamp column and with a literal, `CASE`, and `coalesce`.
- The criterion benchmark now passes the new return type.
With this fix, `timestamp_seconds` no longer triggers the `CASE`/`coalesce`
panic in #6327. `date_trunc` in an `Etc/UTC` session still triggers it (#6330),
so #6327 stays open.
## How are these changes tested?
- I ran the new SQL file against an unfixed build of `main` first. It failed
in all three sessions: `hour` and `minute` were wrong in Los Angeles and
Kolkata, and UTC failed with `Invalid comparison operation: Timestamp(µs) ==
Timestamp(µs, "UTC")`.
- With this change, the new file and the existing `timestamp_seconds` files
pass on Spark 4.1 and 3.5.
- On Spark 4.1, all 148 `expressions/datetime/` SQL file tests and
`CometTemporalExpressionSuite` pass.
- The new Rust unit tests pass, and `cargo clippy --all-targets -- -D
warnings` is clean for `datafusion-comet-spark-expr`.
--
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]
