Commit 19e6502
## Summary
- Add T-vs-space separator validation in the incompatible date formats
path to reject inputs where cuDF would incorrectly parse 'T' as a space.
- Remove the `.exclude(...)` for the `SPARK-33498` test case in
`RapidsTestSettings.scala`, re-enabling the test.
## Root Cause
When `incompatibleDateFormats.enabled=true`, the format `"yyyy-MM-dd
HH:mm:ss.SSS"` was not in `CORRECTED_COMPATIBLE_FORMATS` and went
through the incompatible path. The incompatible path assumed all
non-null inputs are valid. cuDF is lenient about `T` vs space
separators, so input `"2020-01-27T20:06:11.847"` with format
`"yyyy-MM-dd HH:mm:ss.SSS"` was parsed successfully on GPU instead of
returning `null` (which is what CPU does when the format specifies a
space separator).
### Why CPU returns null
On CPU, Spark uses Java's `DateTimeFormatter` (in CORRECTED/EXCEPTION
mode) or `SimpleDateFormat` (in LEGACY mode). These formatters perform
**strict character-by-character matching** against the format pattern.
The space between `dd` and `HH` in `"yyyy-MM-dd HH:mm:ss.SSS"` is a
literal character — when parsing `"2020-01-27T20:06:11.847"`, the
formatter expects a space at position 10 but finds `T`, so parsing fails
and returns `null` (non-ANSI) or throws an exception (ANSI).
### Why GPU returned a non-null value
cuDF's timestamp parser treats `T` and space as **interchangeable
date-time separators** (for ISO 8601 compatibility), regardless of what
the format string specifies. So `"2020-01-27T20:06:11.847"` parses
successfully even when the format says space.
The incompatible path in `isTimestamp()` previously returned all-true
without any validation, letting cuDF's lenient behavior through.
## Fix
Add targeted validation in the incompatible code path of
`GpuToTimestamp.isTimestamp()`:
1. **T-vs-space check**: For formats starting with `%Y-%m-%d %H` or
`%Y/%m/%d %H`, reject inputs that have `T` at position 10 (where the
format expects a space). This uses a regex `\A.{10}T` to detect the
mismatch.
2. **cuDF isTimestamp check**: Also apply cuDF's own `isTimestamp`
validation to reject truly unparseable strings (e.g., `"Unparseable"` →
`null`).
### Why NOT add to CORRECTED_COMPATIBLE_FORMATS
An earlier approach added the format to `CORRECTED_COMPATIBLE_FORMATS`
with a strict regex. This fixed the original issue but introduced a
regression in `ParseDateTimeSuite`:
- Adding to `CORRECTED_COMPATIBLE_FORMATS` makes the format a "certified
compatible" format, meaning it **always goes to GPU** regardless of
`incompatibleDateFormats`.
- cuDF's `%3f` requires **exactly 3 fractional digits**, but CPU's `SSS`
accepts **1-3 digits** (Java's `DateTimeFormatterBuilder` uses
`appendFraction(..., minWidth=1, maxWidth=3, ...)`).
- Inputs like `"1999-12-31 11:59:59.9"` (1 digit) were rejected by both
the regex (`\.\d{3}` requires 3 digits) and the length check
(`sparkFormat.length == 23` but input is 21 chars), returning `null` on
GPU while CPU returned a valid timestamp.
By keeping the format on the incompatible path, it still **falls back to
CPU** when `incompatibleDateFormats` is not enabled, preserving correct
handling of variable-length fractional seconds.
### Validation examples
| Input | T-vs-space check | cuDF isTimestamp | Combined `isTs` | Result
|
|---|---|---|---|---|
| `2020-01-27T20:06:11.847` | position 10 is T → **reject** | (n/a) |
false | **null** ✓ |
| `2020-01-27 20:06:11.847` | position 10 is space → pass | parseable →
true | true | valid timestamp ✓ |
| `Unparseable` | position 10 is not T → pass | not parseable → false |
false | **null** ✓ |
## UT Mapping
| RAPIDS test | Spark original test | Spark source file | Lines | Source
link |
|---|---|---|---|---|
| `SPARK-33498: GetTimestamp,UnixTimestamp,ToUnixTimestamp with
parseError` (inherited in `RapidsDateExpressionsSuite`) | Same |
`sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala`
| 1700-1754 |
[master](https://github.com/apache/spark/blob/master/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala#L1700-L1754)
|
## Test Validation
The existing Spark test `SPARK-33498` is re-enabled by removing its
`.exclude()` entry. It validates `GetTimestamp`, `UnixTimestamp`, and
`ToUnixTimestamp` with parse errors across all `timeParserPolicy` and
`ansiEnabled` combinations.
Both `RapidsDateExpressionsSuite` and `ParseDateTimeSuite` were run to
verify no regressions:
```
mvn package -pl tests -am -Dbuildver=330 \
-Dmaven.repo.local=./.mvn-repo \
-DwildcardSuites=org.apache.spark.sql.rapids.suites.RapidsDateExpressionsSuite,com.nvidia.spark.rapids.ParseDateTimeSuite \
-Drapids.test.gpu.allocFraction=0.3 \
-Drapids.test.gpu.maxAllocFraction=0.3 \
-Drapids.test.gpu.minAllocFraction=0
```
Result: `Tests: succeeded 87, failed 0, canceled 0, ignored 1, pending
0` — BUILD SUCCESS
### Performance
Cold-path change only. The validation is added to the incompatible code
path in `isTimestamp()`, which only executes when
`incompatibleDateFormats.enabled=true` and the format is not in
`CORRECTED_COMPATIBLE_FORMATS`. The overhead is one regex match
(`\A.{10}T`) and one cuDF `isTimestamp` call per batch — both
lightweight columnar operations. The cuDF `isTimestamp` call was already
being performed downstream in the compatible path; this just moves it
earlier for incompatible formats with date-time separators. No hot-path
modifications, no new allocations in the per-row data processing loop.
### Checklists
- [ ] This PR has added documentation for new or modified features or
behaviors.
- [x] This PR has added new tests or modified existing tests to cover
new code paths.
- [x] Performance testing has been performed and its results are added
in the PR description. Or, an issue has been filed with a link in the PR
description.
Closes #13759
---------
Signed-off-by: Allen Xu <allxu@nvidia.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 2387d28 commit 19e6502
2 files changed
Lines changed: 29 additions & 5 deletions
File tree
- sql-plugin/src/main/scala/org/apache/spark/sql/rapids
- tests/src/test/spark330/scala/org/apache/spark/sql/rapids/utils
Lines changed: 29 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
706 | 706 | | |
707 | 707 | | |
708 | 708 | | |
709 | | - | |
710 | | - | |
711 | | - | |
712 | | - | |
| 709 | + | |
| 710 | + | |
| 711 | + | |
| 712 | + | |
| 713 | + | |
| 714 | + | |
| 715 | + | |
| 716 | + | |
| 717 | + | |
| 718 | + | |
| 719 | + | |
| 720 | + | |
| 721 | + | |
| 722 | + | |
| 723 | + | |
| 724 | + | |
| 725 | + | |
| 726 | + | |
| 727 | + | |
| 728 | + | |
| 729 | + | |
| 730 | + | |
| 731 | + | |
| 732 | + | |
| 733 | + | |
| 734 | + | |
| 735 | + | |
| 736 | + | |
| 737 | + | |
713 | 738 | | |
714 | 739 | | |
715 | 740 | | |
| |||
Lines changed: 0 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
114 | 114 | | |
115 | 115 | | |
116 | 116 | | |
117 | | - | |
118 | 117 | | |
119 | 118 | | |
120 | 119 | | |
| |||
0 commit comments