andygrove opened a new pull request, #5150:
URL: https://github.com/apache/datafusion-comet/pull/5150
## Which issue does this PR close?
Part of #5149. This PR covers the first five subtasks of that epic; the
timestamp targets and the latent `to_time.rs` trim are left for follow-ups.
## Rationale for this change
Comet's cast-from-string kernels used four different trim sets, three of
which diverge from Spark. Spark has exactly two trim regimes, and neither of
them trims any non-ASCII whitespace:
| Regime | Trimmed bytes | Cast targets |
| --- | --- | --- |
| `UTF8String.trimAll` | `0x00`-`0x20` and `0x7F` | boolean, byte, short,
int, long, date, timestamp, timestamp_ntz |
| `java.lang.String.trim` | `0x00`-`0x20` | float, double, decimal |
Every affected cast is `Compatible()` in `CometCast.canCastFromString`, so
all of them run natively by default. The divergence went in both directions:
spurious failures where Spark parses fine, and silent wrong results (with the
ANSI error swallowed) where Spark returns `NULL` or raises.
```sql
-- before this PR
SELECT CAST(concat(char(1), 'true') AS boolean); -- Spark: true, Comet:
NULL (throws under ANSI)
SELECT CAST(concat(char(1), '123') AS int); -- Spark: 123, Comet:
NULL (throws under ANSI)
SELECT CAST(concat(' ', 'true') AS boolean); -- Spark: NULL, Comet: true
SELECT CAST(concat(' ', '1.5') AS double); -- Spark: NULL, Comet: 1.5
```
## What changes are included in this PR?
- New `native/spark-expr/src/conversion_funcs/trim.rs` with the two regimes
as explicit, documented helpers (`trim_all` / `trim_all_bytes` and
`trim_java_string`), unit-tested against the byte sets themselves rather than
against cast results.
- `CAST(string AS boolean)` uses the `trimAll` set. The keyword comparison
is now allocation-free (`eq_ignore_ascii_case`) instead of
`to_ascii_lowercase()` per row.
- `CAST(string AS byte/short/int/long)` uses the `trimAll` set in all three
eval modes. `<[u8]>::trim_ascii` was excluding `0x0B`, which Spark trims.
- `CAST(string AS float/double)` and `CAST(string AS decimal)` use the
`String.trim` set, leaving `0x7F` untrimmed and preserving the existing NUL
handling (NUL is in that set).
- `date_parser` was already correct; it now shares the predicate instead of
recomputing it, so there is one definition of the `trimAll` set.
- Docs: the compatibility page documents both regimes; the boolean entry in
the known-divergences list is replaced by an entry for the remaining timestamp
divergence.
Not in this PR (tracked on #5149): `CAST(string AS timestamp)` /
`timestamp_ntz`, including `timestamp_parser`'s Spark-4 leading-whitespace
gate, and the latent `to_time.rs` trim.
## How are these changes tested?
- Rust: the trim helpers are tested byte-by-byte against Java's
`Character.isWhitespace`/`isISOControl` definitions, plus a parity matrix
(leading / trailing / both / interior position × all three eval modes) for
every fixed cast target. Reverting any one of the four kernel changes fails the
corresponding matrix test.
- Scala: three new `CometCastSuite` tests run the same codepoint matrix
through `castTest`, so real Spark is the oracle for values, `NULL`s and ANSI
error messages.
- Full `CometCastSuite` and the `cast` SQL-file tests pass on Spark 3.5.
--
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]
