david-mollitor-db commented on PR #59213: URL: https://github.com/apache/spark/pull/59213#issuecomment-6025610555
@pratham76 Thanks for the review, and for comparing this with #59233. I agree that's the trade-off: #59233 is simpler because it doesn't copy `String.split`'s single-char check, and this PR is faster for single-char delimiters. For this one I'd take the speed, because single-char delimiters (`split(s, ',')`, `'-'`, `' '`, `'\\.'`, `'\\|'`) are the common case for `split`. On master those don't compile a regex at all, because `String.split` handles them with `indexOf`. Sending them through a precompiled `Pattern` would make the most common case slower to speed up the less common ones. A quick local microbenchmark of just the split call (1M strings like `123-246`, median ns/row): | Delimiter | JDK | master | this PR | always `Pattern.split` | |---|---|---|---|---| | `'-'` | 17 | 36.6 | 44.2 | 66.5 | | `'-'` | 25 | 40.1 | 46.0 | 57.9 | | `'[-]'` | 17 | 138.8 | 68.7 | 68.7 | | `'[-]'` | 25 | 126.9 | 58.2 | 58.2 | On the copied check: if a future JDK changes its own check, only speed is affected, not results. Outside the fast path `String.split` is `Pattern.compile(regex).split(this, limit)`, so the two calls always return the same thing. That's also why I'd keep the check in this PR rather than a follow-up: without it, caching the pattern on its own makes single-char delimiters slower. -- 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]
