apache / apache/datafusion-comet

to_time / try_to_time: native parser rejects 'T12' and '12:30:45.' which Spark accepts

Geschlossen
#5,366 2 Kommentare 0 Reaktionen 1 zugewiesene Person Beansprucht von @peterxcli Auf GitHub ansehen
area:expressions bug correctness priority:critical temporal expressions
Vorherrschende Sprache
Scala
Sterne
1.3k
Forks
375
Ø Merge
2 T. 10 Std.
Gemergte PRs (30 T.)
231

Beschreibung

## Describe the bug

Comet's native `to_time` / `try_to_time` parser (`native/spark-expr/src/datetime_funcs/to_time.rs`) diverges from Spark 4.1's `SparkDateTimeUtils.stringToTime` on three inputs that have nothing to do with whitespace trimming. In the first two cases Spark returns a value and Comet raises `The input string '...' cannot be parsed to a TIME value` (or returns `NULL` for `try_to_time`), so enabling Comet turns a succeeding query into a failing one.

These were found while reviewing #5364, which fixes a separate whitespace-trimming divergence in the same function. They are pre-existing and are not caused by that PR.

### 1. `T`-prefixed hour with no minute component

```sql
SELECT to_time('T12'); -- Spark: 12:00:00 Comet: error
SELECT to_time('T1'); -- Spark: 01:00:00 Comet: error
SELECT to_time('T12 AM'); -- Spark: 00:00:00 Comet: error
```

In `parseTimestampString`, the `T` branch (`j == 0 && b == 'T'`) sets `justTime = true` and advances `i += 3`, so the following digits are accumulated directly into `segments(3)` (the hour). The loop then ends and `isValidDigits(3, 2)` passes, giving `hr = 12, min = 0, sec = 0`.

Comet's `parse_time_components` skips an optional leading `T` but then unconditionally requires a `:` after the hour:

https://github.com/apache/datafusion-comet/blob/main/native/spark-expr/src/datetime_funcs/to_time.rs#L188-L190

### 2. Trailing `.` with no fractional digits

```sql
SELECT to_time('12:30:45.'); -- Spark: 12:30:45 Comet: error
```

Spark's `isValidDigits` short-circuits on `segment == 6`, so the fractional-second segment is allowed to have zero digits. Comet's `parse_fractional` returns `None` when `count == 0`.

### 3. Fractional digits 7 through 9 are dropped

```sql
SELECT to_time('12:30:45.1234567');
```

Spark keeps fractional digits 7-9 as a sub-microsecond remainder in `segments(9)` and folds them into `nanoOfSecond`, storing `45045123456700` nanos. Comet's `parse_fractional` truncates at 6 digits and stores `45045123456000`.

This one may not be user-observable: `to_time` produces `TIME(6)`, and the existing test at `spark/src/test/resources/sql-tests/expressions/datetime/to_time.sql` (`SELECT to_time('00:00:00.1234567')`) passes today because `TIME(6)` formatting truncates the difference away. It needs a check for whether a widening cast or `extract(second from ...)` can expose the stored nanos before deciding whether to fix it.

## Steps to reproduce

Requires Spark 4.1 with `spark.sql.timeType.enabled=true`.

```sql
SELECT to_time('T12'), to_time('12:30:45.');
```

## Expected behavior

Comet should return the same values Spark does.

## Additional context

Found with a differential harness that ports Spark 4.1's `stringToTime` and `parseTimestampString` to Rust and compares them against `string_to_time` over ~107k generated inputs (30 core time strings crossed with all 34 `trimAll` bytes and seven Unicode whitespace codepoints in leading, trailing, doubled, interior and pre-suffix positions). After #5364 lands, these three are the only remaining divergence classes in that corpus.

Beitragsleitfaden

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.