Github user rednaxelafx commented on the issue: https://github.com/apache/spark/pull/20700 Aha! Thanks @kiszk san for working on this! I really wanted the stateless methods to be extracted so that I can use more utils without having to pass around a `CodegenContext` for no good. I was actually working on the exact same thing last week but got carried away on other tasks so my patch is still WIP. For those curious, here's my WIP patch: https://github.com/rednaxelafx/apache-spark/commit/249fb93c5cacc35d53e6b0f972b139d27ef5d720 I was hoping we can cover the ones I extracted here: https://github.com/rednaxelafx/apache-spark/commit/249fb93c5cacc35d53e6b0f972b139d27ef5d720#diff-8bcc5aea39c73d4bf38aef6f6951d42cR1201 Your PR and my version extracted mostly the same fields/methods, which is great for me that I'll just help get this PR in instead of having to send mine out. The approach is different, though. I'd like to have a discussion on the tradeoffs you had in mind when you picked your approach. I initially did something similar, which is to extract stateless methods into companion object methods and change the original one to delegate to the companion one. But the old instance methods were a bad smell to me (passing in a instance when it shouldn't need the instance), so since we're at it, I thought why not just completely remove that bad smell once and for all. So I deleted the original instance methods, and yes that made the diff huge because all use sites of those fields/methods have to be touched to switch to using the new version. @kiszk WDYT? Were you intending to minimize the diff but still want to be able to access the stateless util methods as standalone functions?
--- --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org