apache / apache/datafusion-comet
[EPIC] cast from string: trim semantics diverge from Spark across all numeric, datetime and boolean targets
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
### Describe the bug
Comet's cast-from-string kernels do not reproduce Spark's whitespace-trimming semantics. This is currently tracked as a boolean-only problem in #4959, but an empirical audit of every string cast target shows that **7 of the 8 supported targets diverge, in both directions**, and the direction not covered by #4959 (Comet returning a non-NULL value where Spark returns NULL / throws) is the more severe one.
This epic supersedes #4959.
#### Verification method
- **Spark ground truth**: a Java probe run directly against the Spark 3.5.5 jars, exercising the exact primitives each cast target uses (`UTF8String.trimAll`, `UTF8String.toInt`/`toLong`, `Double.parseDouble`, `new BigDecimal(String.trim)`, `DateTimeUtils.stringToDate`/`stringToTimestamp`/`stringToTimestampWithoutTimeZone`), over a 25-codepoint matrix in leading and trailing position.
- **Comet**: the same matrix through `spark_cast` in `datafusion-comet-spark-expr`, in both legacy and ANSI eval mode.
- The relevant Spark code paths are identical in 3.5.8 and 4.1.1.
### Spark has two trim regimes, not one
| Regime | Trim set | Cast targets |
| --- | --- | --- |
| `UTF8String.trimAll` / `SparkDateTimeUtils.getTrimmedStart`+`getTrimmedEnd` | bytes `0x00`-`0x20` **and `0x7F`** | boolean, byte, short, int, long, date, timestamp, timestamp_ntz |
| Java `String.trim` | chars `U+0000`-`U+0020` (**not `0x7F`**) | float, double, decimal |
Neither regime trims **any** non-ASCII whitespace. `U+0085`, `U+00A0`, `U+1680`, `U+2000`-`U+200A`, `U+2028`, `U+2029`, `U+202F`, `U+205F` and `U+3000` all cause Spark to return NULL (or throw under ANSI) for every target.
Sources: `UTF8String.java` (`isWhitespaceOrISOControl`, `trimAll`, `toInt`, `toLong`), `StringUtils.isTrueString`/`isFalseString`, `Decimal.stringToJavaBigDecimal`, `Cast.castToDouble`/`castToFloat`, `SparkDateTimeUtils.stringToDate`/`parseTimestampString`.
### Comet uses four different trim sets, three of them wrong
| Cast target | Comet trim | Under-trims: Comet NULL / throws, Spark parses | Over-trims: Comet returns a value, Spark NULLs / throws |
| --- | --- | --- | --- |
| boolean | `str::trim` | `0x00`-`0x08`, `0x0E`-`0x1F`, `0x7F` | **all non-ASCII whitespace** |
| byte / short / int / long | `<[u8]>::trim_ascii` | `0x00`-`0x08`, **`0x0B`**, `0x0E`-`0x1F`, `0x7F` | - |
| float / double | `str::trim` | `0x00`-`0x08`, `0x0E`-`0x1F` | **all non-ASCII whitespace** |
| decimal | ascii-ws + NUL | `0x01`-`0x08`, **`0x0B`**, `0x0E`-`0x1F` | - |
| date | `is_ascii_whitespace() \|\| is_ascii_control()` | - | - |
| timestamp | `str::trim` | `0x00`-`0x08`, `0x0E`-`0x1F`, `0x7F` | **all non-ASCII whitespace** |
| timestamp_ntz | `str::trim` | `0x00`-`0x08`, `0x0E`-`0x1F`, `0x7F` | **all non-ASCII whitespace** |
`date_parser` is the only correct path: its local `is_whitespace_or_iso_control` (`native/spark-expr/src/conversion_funcs/string.rs`) computes `is_ascii_whitespace() || is_ascii_control()`, which is exactly Spark's `trimAll` set. It is the model for the fix.
Two details that are easy to get wrong:
1. Rust's `u8::is_ascii_whitespace` **excludes `0x0B`** (vertical tab) while `str::trim` includes it. That is why the int and decimal paths have an extra failing byte that the boolean and float paths do not.
2. `0x7F` **must** be trimmed for boolean / int / date / timestamp but **must not** be trimmed for float / double / decimal. A single shared helper applied uniformly would introduce a new divergence in the float and decimal paths.
### Severity
Every one of these casts is `Compatible()` in `CometCast.canCastFromString`, so they all run natively by default with no `allowIncompat` gate.
Measured ANSI-mode behavior:
| Input | Spark | Comet |
| --- | --- | --- |
| `"\x01true"` AS boolean | `true` | **throws** `CAST_INVALID_INPUT` |
| `"\x7F123"` AS int | `123` | **throws** |
| `" true"` AS boolean | **throws** | `true` |
| `" 1.5"` AS double | **throws** | `1.5` |
| `" 2020-01-01 10:00:00"` AS timestamp | **throws** | a timestamp |
So the impact is spurious query failures in one direction, and silent wrong results with a swallowed ANSI error in the other. The over-trim direction is the `priority:critical` part and is currently documented nowhere: the entry in `docs/source/user-guide/latest/compatibility/index.md` covers only the boolean under-trim.
### Also in scope
- `native/spark-expr/src/datetime_funcs/to_time.rs` uses the same `str::trim`. **Latent only**: `spark_to_time` is exported from the crate but is not registered in `jni_api` and has no serde entry, so it is unreachable from Spark today. Spark's `stringToTime` is also subtler than the others (`trimRight` first, for AM/PM suffix detection, then `parseTimestampString`'s trimAll set), so porting it needs its own analysis. Should be fixed before `to_time` is wired up, and coordinated with #4288.
- `timestamp_parser`'s Spark-4 leading-whitespace gate decides "has leading whitespace" via `value.trim_start()`, so it inherits the same wrong notion of whitespace and needs updating alongside the trim itself.
### Adjacent, not part of this epic
`native/spark-expr/src/csv_funcs/to_csv.rs` implements `ignoreLeadingWhiteSpace` / `ignoreTrailingWhiteSpace` with `trim_start` / `trim_end`. Univocity's semantics are `<= ' '`, so this is very likely the same class of bug, but it has a different root cause and was **not verified**. Worth its own issue rather than folding in here.
### Proposed subtasks
- [ ] Introduce two explicit, documented trim helpers in `conversion_funcs` -- one for the `trimAll` set (bytes `0x00`-`0x20` plus `0x7F`) and one for the Java `String.trim` set (`0x00`-`0x20` only) -- and unit-test them directly against the byte sets rather than against cast results.
- [ ] Fix `CAST(string AS boolean)` to use the trimAll helper (the original #4959 scope, plus the over-trim direction).
- [ ] Fix `CAST(string AS byte/short/int/long)` to use the trimAll helper, in all three eval-mode variants (`legacy`, `ansi`, `try`).
- [ ] Fix `CAST(string AS float/double)` to use the `String.trim` helper, keeping `0x7F` untrimmed.
- [ ] Fix `CAST(string AS decimal)` to use the `String.trim` helper, keeping `0x7F` untrimmed and preserving the existing NUL handling.
- [ ] Fix `CAST(string AS timestamp)` and `CAST(string AS timestamp_ntz)` to use the trimAll helper, including the Spark-4 leading-whitespace gate in `timestamp_parser`.
- [ ] Add a shared parity test covering the full codepoint matrix (leading, trailing, both, and interior positions) for every string cast target, in all eval modes.
- [ ] Fix the latent `to_time.rs` trim, or record the divergence on #4288 so it is handled when `to_time` is wired up.
- [ ] Update `docs/source/user-guide/latest/compatibility/index.md` to describe both divergence directions and all affected targets, and remove the entries once each target is fixed.
- [ ] File a separate issue for the `to_csv` `ignoreLeading/TrailingWhiteSpace` trimming, after confirming univocity's semantics.
### Steps to reproduce
```sql
-- 0x01 is an ASCII control byte; Spark trims it, Comet does not
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)
-- U+3000 is Unicode whitespace but not ASCII; Spark does not trim it, Comet does
SELECT CAST(concat(' ', 'true') AS boolean); -- Spark: NULL, Comet: true
SELECT CAST(concat(' ', '1.5') AS double); -- Spark: NULL, Comet: 1.5
```
### Expected behavior
Comet's string casts should trim exactly the byte set that the corresponding Spark cast path trims: bytes `0x00`-`0x20` plus `0x7F` for boolean / integral / date / timestamp / timestamp_ntz, and bytes `0x00`-`0x20` only for float / double / decimal. No non-ASCII whitespace should be trimmed by any of them.
### Additional context
Originally reported for boolean only in #4959, which came out of the review of #4914. That PR deliberately left the pre-existing trim behavior alone to stay a pure performance change.
Contributor guide
Assessment
This issue has not been assessed yet.