andygrove opened a new pull request, #6345:
URL: https://github.com/apache/datafusion-comet/pull/6345
## Which issue does this PR close?
Closes #6330.
## Rationale for this change
Since #4761, `TimestampTruncExpr` has declared and emitted
`Timestamp(Microsecond, <session timezone>)`. Everywhere else in a native plan,
`TimestampType` is labelled `"UTC"`, and Arrow's comparison kernels require
identical types, so comparing a truncated timestamp with any other timestamp
failed.
That hits the default configuration. `CometTruncTimestamp` treats `Etc/UTC`
as UTC and runs natively there. So in an `Etc/UTC` session, `WHERE
date_trunc('DAY', ts) >= TIMESTAMP'...'` failed with `Invalid comparison
operation: Timestamp(µs, "Etc/UTC") >= Timestamp(µs, "UTC")`. `Etc/UTC` is the
JVM default, and so Spark's default session timezone, on Ubuntu and Debian
images. With `allowIncompatible=true`, every other non-UTC zone failed the same
way. `CASE` and `coalesce` panicked instead (#6327).
## What changes are included in this PR?
- **The fix.** `TimestampTruncExpr` still truncates in the session timezone,
but its result now keeps the input's timezone label. `data_type()` returns the
child's type, and `evaluate` relabels the kernel's output with an Arrow cast,
which changes the label but not the values. Dictionary inputs keep their label
too. Declared and actual types still agree, so the `RowConverter` mismatch that
#4761 fixed stays fixed.
- **Rust unit tests.** They check the declared and actual types, and the
truncated value, for plain and dictionary inputs. They use a session timezone
with a half-hour offset.
- **A new SQL file test, `trunc_timestamp_label.sql`.** It runs in `UTC`,
`Etc/UTC`, `Asia/Tokyo` and `Asia/Kolkata` with `allowIncompatible=true`. It
compares the result with a timestamp column and with a literal, and uses it in
`BETWEEN`, `<=>`, `nullif`, `CASE`, `coalesce` and a join condition. None of
these zones has DST transitions, which keeps the test clear of #5633.
With this fix, `date_trunc` no longer triggers #6327's panic. #6327 stays
open for the planner-side fix. The `UTC`/`Etc/UTC` equivalence that #5556 added
to the Python runner is no longer needed for `date_trunc`. It's harmless, so I
left it alone.
## How are these changes tested?
- **Unfixed `main`.** I ran the new SQL file against unfixed `main` first.
It passed in `UTC` and failed in the other three zones with `Invalid comparison
operation: Timestamp(µs, "Etc/UTC") == Timestamp(µs, "UTC")` and its Tokyo and
Kolkata equivalents.
- **This branch.**
- The new file passes in all four zones.
- Every `expressions/datetime/` SQL file test and
`CometTemporalExpressionSuite` pass on Spark 4.1.
- The new and existing truncation unit tests pass.
- `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]
