andygrove opened a new pull request, #6445:
URL: https://github.com/apache/datafusion-comet/pull/6445
## Which issue does this PR close?
Closes #5165.
Part of #5149. Timestamp and timestamp_ntz are the last cast targets in that
epic; the one item left there is filing a separate issue for `to_csv`'s
whitespace options.
## Rationale for this change
`CAST(string AS TIMESTAMP)` and `CAST(string AS TIMESTAMP_NTZ)` trimmed
their input with Rust's `str::trim`, which strips Unicode whitespace. Spark
trims the `UTF8String.trimAll` byte set (`0x00`-`0x20` and `0x7F`) through
`SparkDateTimeUtils.getTrimmedStart` / `getTrimmedEnd`, and never trims
non-ASCII whitespace. So Comet diverged in both directions:
| Input | Spark
| Comet before this PR |
| ----------------------------------------------- |
------------------------------------------ |
------------------------------------------ |
| `'2020-01-01 12:34:56'` with a leading `0x01` | `2020-01-01 12:34:56`
| `NULL`, or `CAST_INVALID_INPUT` under ANSI |
| `'2020-01-01 12:34:56'` with a leading `U+3000` | `NULL`, or
`CAST_INVALID_INPUT` under ANSI | `2020-01-01 12:34:56` |
The second row is a silently wrong result, and under ANSI it swallows the
error Spark raises.
While checking this I also found that an empty or all-whitespace string
returned `NULL` under ANSI for both targets. Spark raises `CAST_INVALID_INPUT`
there: `parseTimestampString` finds no segments, and `stringToTimestampAnsi` /
`stringToTimestampWithoutTimeZoneAnsi` throw when the parse returns `None`. I
confirmed this against the Spark 4.1.1 jars; 3.4 and 3.5 take the same path.
This supersedes the trimming half of #5172. The other half of #5165, a
leading `+` returning `NULL` under ANSI, was already fixed on main by #5858.
## What changes are included in this PR?
- `timestamp_parser` and `timestamp_ntz_parser` trim with the existing
`trim_all` helpers from `conversion_funcs::trim` instead of `str::trim`.
- Spark 4.0+ rejects padding before a time-only `T`, because it only treats
the `T` as a time-only marker at raw byte 0. That check now uses the trimmed
start offset instead of `trim_start()`, so a leading ISO control character
counts as padding there too: `\u0001T2` is `NULL` on 4.0+ and valid on 3.x, as
in Spark.
- `cast_utf8_to_timestamp!` no longer calls `trim_end()` on each value
before parsing. The parsers do all of the trimming now, and that `trim_end()`
would still have stripped trailing non-ASCII whitespace.
- A value that trims to nothing raises `CAST_INVALID_INPUT` under ANSI for
both targets. It still returns `NULL` in legacy and try mode.
- The compatibility guide no longer lists the divergence, and its trim table
now includes `TIMESTAMP_NTZ`.
## How are these changes tested?
- Rust: `assert_trim_parity` now covers timestamp and timestamp_ntz. It runs
the full codepoint matrix (every byte `0x00`-`0x20`, `0x7F`, and eleven
non-ASCII whitespace codepoints, in leading, trailing, both-ends, interior and
padding-only position, plus the empty string) in all three eval modes. This
replaces the test that pinned the old divergence.
`test_leading_whitespace_t_hm` gains control-character and `U+3000` cases for
the Spark 4 check.
- `CometNativeCastSuite`, with Spark as the oracle:
- whitespace trim parity tests for timestamp, timestamp_ntz and date (date
already matched but had only Rust coverage);
- a per-value ANSI test for inputs that trim to nothing, since the
batch-wide ANSI comparison in `castTest` only checks that both sides throw
somewhere in the batch;
- control-character and `U+3000` cases in the T-hour-only whitespace test.
- `cast_string_trim.sql` gains timestamp and timestamp_ntz columns.
Locally: `datafusion-comet-spark-expr` tests and clippy pass; the full
`CometNativeCastSuite` passes on the default Spark 4.1 profile (189 succeeded,
0 failed); its whitespace tests also pass on `-Pspark-3.5`, which covers the
Spark 3.x side of the time-only `T` check; and the `expressions/cast` SQL file
tests pass.
--
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]
