david-mollitor-db opened a new pull request, #58834:
URL: https://github.com/apache/spark/pull/58834
### What changes were proposed in this pull request?
JSON, CSV, and XML schema inference probe whether a string value is a `TIME`
by calling the
throwing `TimeFormatter.parse` inside `allCatch opt`:
```scala
if (isTimeTypeEnabled && (allCatch opt
timeFormatter.parse(field)).isDefined) {
TimeType(TimeType.DEFAULT_PRECISION)
}
```
For the default (pattern-less) formatter, `parse` calls
`SparkDateTimeUtils.stringToTimeAnsi`, which
throws a `SparkDateTimeException` (filling in a stack trace) for every value
that is not a time.
This PR adds a non-throwing `TimeFormatter.parseOptional`, mirroring
`TimestampFormatter`:
- trait default: `try Some(parse(s)) catch { case _: Exception => None }`;
- `Iso8601TimeFormatter`: an exception-free implementation using
`DateTimeFormatter.parseUnresolved`
with a `ParsePosition`, requiring the whole input to be consumed (as in
`Iso8601TimestampFormatter`);
- `DefaultTimeFormatter`: delegates to `SparkDateTimeUtils.stringToTime`
(already `Option[Long]`).
and switches the `TimeType` inference probe in `JsonInferSchema`,
`CSVInferSchema`, and
`XmlInferSchema` to `timeFormatter.parseOptional(field).isDefined`.
### Why are the changes needed?
Inferring a non-time column throws and catches one exception per row. This
is exactly the cost that
SPARK-39193 / SPARK-39280 / SPARK-39281 removed for timestamp and date
inference by adding
`parseOptional` (that work reported exception handling as more than 90% of
type-inference time).
`TimeType` was added later and never got a `parseOptional`, so it is the
only datetime formatter
still using the throwing probe — the timestamp probe immediately below the
time probe already uses
`parseOptional`.
Profiling `JsonBenchmark` under JFR (`settings=profile`):
- allocation samples through `SparkDateTimeUtils.stringToTimeAnsi` (the
throwing time probe):
~64,400 → 0;
- `java.lang.Throwable.fillInStackTrace`: ~1.7% of sampled allocation →
absent;
- total sampled allocation: about 23% lower.
No new allocation is introduced: the default inference path (no custom
`timeFormat`) uses
`stringToTime` and allocates no `ParsePosition`; the `Iso8601` path
allocates only a small
`ParsePosition` in place of the former exception and stack trace.
### Does this PR introduce _any_ user-facing change?
No. `parseOptional` accepts exactly what `parse` accepts (full-string
consumption), so inference
results are unchanged.
### How was this patch tested?
`TimeFormatterSuite` gains a `parseOptional` test (valid input returns
`Some`, invalid returns
`None`, prefix/partial match returns `None`). Existing inference tests pass
unchanged:
```
build/sbt 'catalyst/testOnly *TimeFormatterSuite *CSVInferSchemaSuite'
build/sbt 'sql/testOnly *JsonSuite *JsonV1Suite *JsonV2Suite *CSVSuite
*CSVv1Suite *CSVv2Suite *XmlSuite'
```
All 902 tests pass.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Isaac
This pull request and its description were written by Isaac.
--
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]
