viirya commented on PR #57137: URL: https://github.com/apache/spark/pull/57137#issuecomment-4931247811
Closing this PR. After investigating your objections in depth, you are right on both counts, and my earlier reply to you was incorrect — I should have verified before arguing back. You called this correctly before we did. What we found when we re-measured properly: 1. **For identity children, the existing code already avoids per-row work** — the converter is `None` and results go straight to `pa.array`, exactly as you said. My "defensive copy" claim was wrong for those cases. 2. **For string-bearing types, the per-row converter is `str()` coercion** — a semantic contract (e.g. a UDF returning `bytes` for a string field yields `"b'x'"` today), which a bulk path cannot safely skip: `pa.array` would silently decode the bytes instead, with no error to trigger a fallback. 3. **Consequently, with the identity-only guard this PR shipped with, the bulk paths never actually engage for realistic types.** A strategy probe on the live worker shows every benchmark case takes the fallback, which is equivalent to the existing code. 4. The end-to-end deltas I previously cited were measurement artifacts: stage-level instrumentation shows the difference spread uniformly across stages whose code is byte-identical (including the UDF invocation itself), so it cannot be attributed to this change. I withdraw those numbers. Apologies for the review time this consumed. The corrective takeaway on our side: performance claims must come with verification that the intended fast path actually executes and that the gain is localized to it — output equivalence and a benchmark delta alone are not evidence. -- 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]
