andygrove opened a new issue, #5149:
URL: https://github.com/apache/datafusion-comet/issues/5149
### 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.
--
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]
