gaogaotiantian commented on PR #57911: URL: https://github.com/apache/spark/pull/57911#issuecomment-5249347372
I don't fully buy this. I think the benchmark diff is purely caused by re-ordering the if statements. You are running benchmarks on cases where the fast path is taken (fast because you just moved it to the top). What about the old benchmark? Could you check the "slow" path when the input is not `str`? For example, what if they are all `None`? `str(s) is s` and `bytes(b) is b` (when `s` and `b` are `str` and `bytes` respectively) - there's no copy involved. I believe this PR simply makes some cases faster and some slower by moving around stuff. We can't add a bunch of benchmarks that are supposed to be better by reordering to support the change. Also for every `type()` check introduced, we add an extra function call overhead. I'm not saying we should not prioritize `str -> str` path. It might be real that this is a more common case. But this PR is a heuristic, not an optimization. We should at least be clear about that and leave it in the description and comments - that we believe in most UDF cases, users return `str` when they use `StringType`, so we do a fast path check first. Not sure if we have enough support data for that. However, I'm a bit skeptical for this kind of heuristic in general. This works slightly better for some cases and slightly worse in others. We could potentially be swamped in a discussion about "how our users use our library". -- 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]
