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