adgubrud opened a new pull request, #59686: URL: https://github.com/apache/spark/pull/59686
### What changes were proposed in this pull request? Backport of [SPARK-48143](https://issues.apache.org/jira/browse/SPARK-48143) (commit a6632ffa16f6907eba96e745920d571924bf4b63) to `branch-3.5`. The upstream commit introduces `LazyBadRecordCauseWrapper` so the expensive user-facing exception is only constructed when a consumer actually needs it, and suppresses stack-trace collection on `BadRecordException`, which is used purely for control flow. This is not a clean cherry-pick. Three adaptations were required for `branch-3.5`: 1. `QueryExecutionErrors.malformedRecordsDetectedInRecordParsingError` takes `e: BadRecordException` on 3.5 and `e: Throwable` on 4.0. Widened to `Throwable` to match 4.0 so the unwrapped lazy cause can be passed. `e` is only used as `cause = e`, so the body is unchanged and all existing callers still compile. 2. The `BadRecordException` import in `QueryExecutionErrors.scala` becomes unused as a result, which is a fatal warning in Spark's build. Removed, matching 4.0. 3. `FailureSafeParser`'s `FailFastMode` branch differs structurally on 3.5 (it predates the `StringAsDataTypeException` case). The lazy wrapper is unwrapped so the error still reports the concrete reason, but **every other cause continues to pass `e` through unchanged**, preserving the exception-chain shape that 3.5.x already had. 4.0 unwraps in both cases; doing that here breaks `QueryExecutionErrorsSuite`'s `CANNOT_PARSE_DECIMAL` and `JsonSuite`'s SPARK-25040 tests, which walk a fixed number of causes. ### Why are the changes needed? This is a performance regression, not a new optimisation. Commit `6ed03dceafd` (SPARK-41572, shipped in 3.4.0) changed `UnivocityParser.convert` to build the malformed-record exception through the error-class framework and to materialise the bad record with `toString`. Because the exception is constructed per row, reading CSV whose token count does not match the schema became substantially slower from 3.4.0 onward. SPARK-48143 fixed this on master and shipped in 4.0.0, but was filed as an Improvement with `Affects Version/s: 4.0.0`, so it was never backported. Both 3.4.x and 3.5.x still carry the regression; it is reproducible on 3.4.4 and on 3.5.9, the latest release of each line. Measured on an 8 vCPU instance, `local[8]`, JDK 17, TPC-H SF1 `lineitem` (6,001,215 rows) read and written as CSV on local disk. Write-phase elapsed time, median of 3 iterations after a warmup, averaged over 3 rounds: | build | write time | vs 3.3.1 | |---|---|---| | 3.3.1 (before SPARK-41572) | ~6,000 ms | — | | 3.4.4 | ~8,100 ms | +35% | | 3.5.1 | ~8,250 ms | +37% | | **3.5.1 + this patch** | **~3,300 ms** | **-45%** | | 4.0.4 (contains SPARK-48143) | ~3,800 ms | -37% | Three things this shows: 1. The regression appears at 3.4.4, not only 3.5.x, consistent with `6ed03dceafd` having shipped in 3.4.0, and indicating the Affects Version on SPARK-48143 understates the scope. 2. The patch does not merely restore 3.3.1 behaviour, it beats it by 45%, because it also removes the per-row stack-trace capture that 3.3.1 paid via `new RuntimeException`. On this workload the patched 3.5.1 is also faster than 4.0.4. 3. The same pattern reproduces under JDK 8, so it is not JDK-specific. A control confirms the malformed-record branch is the cause rather than CSV writing generally: re-running 3.4.4 against the same data with the trailing delimiter stripped, so the token count matches the schema and the branch is never taken, drops write time from ~8,100 ms to ~3,100 ms (-62%); the same control on JDK 8 gives -55%. With the branch avoided, no version shows the regression. ### Does this PR introduce _any_ user-facing change? Yes, in error reporting: - `BadRecordException` and `LazyBadRecordCauseWrapper` no longer collect stack traces. These are control-flow exceptions; in `PERMISSIVE` mode they are never surfaced. Code that inspects stack traces of malformed-record exceptions would see a change. - In `FAILFAST` mode, when the cause is the new `LazyBadRecordCauseWrapper`, the deferred exception is realised and used as the cause so the error still reports the concrete reason instead of the stackless control-flow wrapper. For every other cause the `BadRecordException` is passed through unchanged, so the existing exception-chain shape is preserved. - `BadRecordException` is shared with `JacksonParser`, so the stack-trace change also applies to JSON malformed-record handling, not only CSV. On `branch-3.5` those are the only two users; the built-in XML data source, which also uses `BadRecordException` on master, does not exist in 3.5.x. This matches the behaviour already shipped in 4.0.0, except for the narrower `FAILFAST` cause handling described above. ### How was this patch tested? - **Full CI on this branch is green**: https://github.com/adgubrud/spark/actions/runs/38802573680 — all jobs passing, including `Build modules: catalyst, hive-thriftserver`, `sql - other tests`, `sql - slow tests` and `sql - extended tests`. - Targeted local run of the suites that exercise the changed code — `JsonV1Suite`, `JsonV2Suite`, `JsonLegacyTimeParserSuite` and `QueryExecutionErrorsSuite` — on JDK 17: **463 passed, 0 failed**. - Those suites were also run against a deliberately-broken variant of this patch (unwrapping the cause in all `FAILFAST` cases, as 4.0 does) to confirm the tests are sensitive to the change: that variant produces 7 failures, which this patch resolves. - Performance measured as described above, against stock 3.5.1, 3.4.4, 3.3.1 and 4.0.4 on the same host, JDK and dataset, plus the arity-matched control for each version. -- 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]
