HyukjinKwon commented on code in PR #56787:
URL: https://github.com/apache/spark/pull/56787#discussion_r3478076057
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/UnivocityParser.scala:
##########
@@ -219,11 +225,11 @@ class UnivocityParser(
}
case _: BooleanType => (d: String) =>
- nullSafeDatum(d, name, nullable, options)(_.toBoolean)
+ nullSafeDatum(d, name, nullable,
options)(retryWithTrim[Boolean](_.toBoolean))
case dt: DecimalType => (d: String) =>
nullSafeDatum(d, name, nullable, options) { datum =>
- Decimal(decimalParser(datum), dt.precision, dt.scale)
+ retryWithTrim(d => Decimal(decimalParser(d), dt.precision,
dt.scale))(datum)
Review Comment:
This decimal fix only applies under `Locale.US`. `retryWithTrim` catches
`NumberFormatException`/`IllegalArgumentException`, which covers the US-locale
`new BigDecimal(...)` path. But for a non-US `options.locale`,
`getDecimalParser` (ExprUtils.scala) uses `DecimalFormat` and throws
`cannotParseDecimalError()` — a `SparkRuntimeException` (extends
`RuntimeException`, not `IllegalArgumentException`). That isn't caught, so the
trimmed retry never runs and `" 1.5 "` still yields `null` under e.g.
`option("locale", "de-DE")`, contradicting the PR's "decimal types … now parsed
correctly."
Consider catching the decimal parser's own failure (e.g. `NonFatal`) for the
decimal case, or trimming before calling `decimalParser`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/UnivocityParser.scala:
##########
@@ -179,6 +179,12 @@ class UnivocityParser(
private val decimalParser = ExprUtils.getDecimalParser(options.locale)
+ private def retryWithTrim[T](f: String => T): String => T = {
Review Comment:
Whitespace tolerance here is implemented as a per-value exception-retry,
which diverges from the float/double cases just below (they get tolerance
natively from the Java parser and never throw). On the PR's own motivating
input `"1, 1"`, every integral/boolean/US-decimal cell now throws and catches a
`NumberFormatException` (stack-trace fill) before the trimmed retry succeeds,
and `retryWithTrim` also allocates a wrapper closure per value (it's invoked
inside the per-value lambda).
For these types an up-front `value.trim` is behaviorally identical —
trimming a valid numeric/boolean is a no-op, and fail-fast still throws on
genuinely bad input — without the per-cell exception or closure. Tradeoff:
up-front trim adds a cheap `trim` scan to clean values too (it returns the same
`String` when there's nothing to strip), in exchange for removing an exception
from the common space-after-delimiter path.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/UnivocityParser.scala:
##########
@@ -179,6 +179,12 @@ class UnivocityParser(
private val decimalParser = ExprUtils.getDecimalParser(options.locale)
+ private def retryWithTrim[T](f: String => T): String => T = {
+ value => try f(value) catch {
+ case _: NumberFormatException | _: IllegalArgumentException =>
f(value.trim)
Review Comment:
`NumberFormatException` is a subclass of `IllegalArgumentException`, so the
first alternative is redundant — `case _: IllegalArgumentException` alone
covers both (and is what `_.toBoolean` actually throws):
```suggestion
case _: IllegalArgumentException => f(value.trim)
```
--
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]
