iemejia commented on PR #55932: URL: https://github.com/apache/spark/pull/55932#issuecomment-5245308934
Thanks @LuciferYang. I've reworked the PR to follow your recommendation and scoped it down to the measurable win plus the correctness fixes. The `readBinary` bulk-slice rewrite has been dropped entirely - both `readBinary` and `readGeoData` are now reverted to their `master` implementations - which removes the `int` overflow, the unguarded `array()` access, and the whole-batch `slice` memcpy in one go, since none of those were justified by wall-time data. The only remaining functional change is `skipBinary`, which keeps the single bulk skip (a consistent 2.00x-2.33x improvement) but is now hardened: lengths are summed into a `long`, each length is validated as non-negative, and the skip is performed via `in.skipFully(...)` wrapped in a `ParquetDecodingException`. This eliminates the infinite CPU spin on an exhausted stream, the silent row drift from negative lengths cancelling positives, and the overflow-to-zero path. I've also added a test that decodes a valid length header over a truncated data region and asserts `skipBinary` fails cleanly instead of hanging. The PR description and the umbrella issue are updated with fresh AMD EPYC 7763 benchmark numbers on the reverted code; `readBinary` now measures identical to the baseline, confirming the revert is a no-op on that path. -- 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]
