andygrove opened a new pull request, #6347:
URL: https://github.com/apache/datafusion-comet/pull/6347
## Which issue does this PR close?
Closes #6327.
## Rationale for this change
`create_case_expr` reconciles the `THEN` and `ELSE` branch types with casts
built by `SparkCastOptions::new_without_timezone`. When two timestamp branches
differed only in their Arrow timezone label, one of them got a
timestamp-to-timestamp cast with an empty timezone, and `array_with_timezone`
panicked on `assert!(!timezone.is_empty())`. `coalesce` goes through the same
path.
Two native expressions currently produce such labels:
- `timestamp_seconds` in any session (#6328)
- `date_trunc` in an `Etc/UTC` session (#6330)
So `CASE WHEN id > 1 THEN timestamp_seconds(id * 3600) ELSE ts END` and
`coalesce(date_trunc('HOUR', ts), ts)` panicked with the default config.
## What changes are included in this PR?
- `create_case_expr` gives the coercion casts the `"UTC"` label that every
`TimestampType` value in a native plan carries. The branches share a Spark
type, so a timestamp difference here is only a label, and Comet's cast turns it
into a relabel.
- `array_with_timezone` returns an error when a timestamp conversion is
given no timezone, instead of asserting. The asserts in
`timestamp_ntz_to_timestamp`, `cast_timestamp_to_ntz` and `pre_timestamp_cast`
were removed, because those functions parse the timezone right away and the
parse already reports an empty one.
- A planner unit test reconciles a `Timestamp(µs, "Etc/UTC")` branch and a
`Timestamp(µs)` branch with a `Timestamp(µs, "UTC")` else branch.
- A `utils` unit test checks that a missing timezone is an error.
#6341 and #6345 fix the two producers, which makes the panic unreachable
from SQL today. This change keeps the next label drift from panicking.
## How are these changes tested?
- **The new planner test.**
- Without the planner change it fails. With only the `array_with_timezone`
change applied, the failure is `Converting a timestamp requires a timezone, but
none was given` rather than a panic.
- With both changes it passes.
- **An end-to-end check on this branch.** Against `main` with only this
change applied, `CASE` over `timestamp_seconds` in UTC, and `CASE` and
`coalesce` over `date_trunc` in `Etc/UTC`, return Spark's results. Without the
change they panic.
- **Existing tests.**
- The planner unit tests and all `datafusion-comet-spark-expr` unit tests
pass.
- Every `expressions/conditional/` and `expressions/datetime/` SQL file
test passes on Spark 4.1.
- `cargo clippy --all-targets -- -D warnings` is clean for both crates.
--
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]
