andygrove opened a new issue, #6328:
URL: https://github.com/apache/datafusion-comet/issues/6328
### Describe the bug
The native `seconds_to_timestamp` function, which Comet uses for
`timestamp_seconds`, declares and returns `Timestamp(Microsecond, None)`
(`native/spark-expr/src/datetime_funcs/seconds_to_timestamp.rs:71`). That is
the Arrow type Comet uses for `TimestampNTZType`. Spark's `SecondsToTimestamp`
returns `TimestampType`, which Comet represents as `Timestamp(Microsecond,
"UTC")` everywhere else. `CometSecondsToTimestamp` goes through
`CometScalarFunction`, which sends no return type, so the planner takes the
UDF's own type and nothing corrects it.
The value is still the right instant, so projecting the result looks fine.
The problem is what happens downstream. Native expressions that consume the
result see a TIMESTAMP_NTZ and treat the micros as wall-clock time, so `hour`,
`CAST(... AS STRING)`, `CAST(... AS DATE)` and `CAST(... AS TIMESTAMP_NTZ)`
silently skip the session timezone. Comparing the result with any other
timestamp fails, even in a UTC session. A `CASE` that mixes it with another
timestamp panics (#6327).
### Steps to reproduce
On `main` at `764936187`, with the default config, on Spark 3.5 and 4.1:
```sql
CREATE TABLE secs USING parquet AS SELECT id, CAST(id * 3600 AS TIMESTAMP)
AS ts FROM range(4);
SET spark.sql.session.timeZone=America/Los_Angeles;
SELECT id, hour(timestamp_seconds(id * 3600 + 1800)),
CAST(timestamp_seconds(id * 3600 + 1800) AS STRING)
FROM secs ORDER BY id;
```
Spark returns `16, 1969-12-31 16:30:00` through `19, 1969-12-31 19:30:00`.
Comet returns `0, 1970-01-01 00:30:00` through `3, 1970-01-01 03:30:00`, which
are the UTC wall-clock values.
```sql
SET spark.sql.session.timeZone=UTC;
SELECT id, timestamp_seconds(id * 3600) = ts FROM secs;
```
Spark returns `true` for every row. Comet fails with `Invalid argument
error: Invalid comparison operation: Timestamp(µs) == Timestamp(µs, "UTC")`.
### Expected behavior
The same results as Spark. The output should be typed
`Timestamp(Microsecond, "UTC")`, like every other `TimestampType` value in a
native plan.
### Additional context
`timestamp_seconds` has been native since #3146, so this is in 1.0.0. Its
SQL tests only run in UTC and only project the result, and the type doesn't
matter there. Could we return `Timestamp(Microsecond, Some("UTC"))` from
`return_type`, stamp the arrays with it, and add a non-UTC test that feeds the
result into `hour`, a cast and a comparison?
--
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]
