HyukjinKwon commented on PR #58796:
URL: https://github.com/apache/spark/pull/58796#issuecomment-5672847822
Reviewed as part of a batch pass (read-only). The optimization itself is
sound — for a 1–9
digit fraction, `raw * nanosMultiplier(9 - len)` reproduces the old
zero-padding exactly and
drops the two throwaway strings. Nice.
One correctness gap, though: the change assumes `nanos.length <= 9`, and
that invariant does
not hold on every path into `parseNanos`.
- The modern parser (`castStringToDTInterval`, `microPattern = (\.\d{1,9})`)
does guarantee
1–9 digits, so that path is fine.
- But `parseNanos` is also called from `parseDayTimeLegacy` (line 507,
`m.group(9)`), whose
regex `dayTimePatternLegacy = "^([+|-])?((\d+)
)?((\d+):)?(\d+):(\d+)(\.(\d+))?$"` captures
the fraction with **`\d+` (unbounded)**, not `\d{1,9}`. This path is
reachable when
`spark.sql.legacy.fromDayTimeString.enabled = true`.
Failure scenario (legacy flag on): a fractional part of 10+ digits whose
numeric value is
`<= 999999999`, i.e. with leading zeros — e.g. `"0 0:0:0.0000000001"`
(fraction
`"0000000001"`, length 10, value 1):
- Old code: `nanos.length (10) < 9` is false, so `alignedStr = nanos`;
`toLongWithRange` = 1
(in range) → returns 0 micros, no error.
- New code: `raw = toLongWithRange(..., "0000000001", 0, 999999999) = 1`
(passes the range
check), then `nanosMultiplier(9 - 10) = nanosMultiplier(-1)` →
`ArrayIndexOutOfBoundsException`
(caught at the `case e: Exception` and rethrown as
`INVALID_INTERVAL_FORMAT.DAY_TIME_PARSING`).
So on the legacy path this input flips from "parses (to 0)" to "throws" —
which contradicts
the "No user-facing change / result is identical" note. (For a 10+ digit
fraction whose value
exceeds 1e9, both old and new throw the same out-of-range error before the
multiplier, so those
are unaffected.)
Minimal fix that preserves the old semantics exactly and avoids the negative
index:
```scala
val nanoSecond =
if (nanos.length >= maxNanosLen) raw
else raw * nanosMultiplier(maxNanosLen - nanos.length)
```
(For `len == 9` this equals `raw * 1`, and for `len > 9` it reproduces the
old
`alignedStr = nanos` branch.) A regression test for the legacy-flag path
would be worth adding.
--
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]
