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
