> If there is an actual issue with case-class inheritance It already brought a lot of issues including subquery / exchange reuse and others. I have already listed a couple of relevant fixes[1] from the Gazelle project. We just kept fixing them until finishing the last one and adding the compilation time enforcer.
Also, you may search for the usage of Spark method "SparkPlan#fastEquals". I assume we don't want that method to give Spark wrong results including the case when WriteFilesExec operators are passing in. Hongze [1] https://github.com/apache/incubator-gluten/pull/5698#issuecomment-2134349360 On Fri, May 31, 2024 at 2:13 PM XiDuo You <[email protected]> wrote: > Hongze, I agree with your point, and I have posted my point in > PR-5698[1]. If there is an actual issue with case-class inheritance, > I'm fine to have some more code and workaround to fix it. > > [1]: https://github.com/apache/incubator-gluten/pull/5698 > > Hongze Zhang <[email protected]> 于2024年5月31日周五 13:07写道： > > > > Thank you for raising the discussion up. > > > > > we find it introduces higher cost than > > expected. It requires more code and workaround to resolve the issue of > > removing case-class inheritance. > > > > I don't think it's true. If you think the previous inheritance approach > of > > VeloxColumnarWriteFilesExec has lower (code-learn / maintenance) cost > than > > the current solution, it's actually because the bad things were hidden. > One > > shouldn't consider the case-class inheritance pattern simple. It's > actually > > something very complicated and dangerous especially when it's used on a > > regular Spark operator. > > > > If some code causes the following: > > > > ``` > > val a = A() > > val b = B() > > > > println(a == b) // "false" > > println(b == a) // "true" > > ``` > > > > Then the code shouldn't be described as "low cost". > > > > I don't like using workarounds either. So to me any solution for the > issue > > can be discussed but I don't think it's acceptable to bring case-class > > inheritance back regarding the healthiness of outcode base. > > > > Best, > > Hongze > > > > On Fri, May 31, 2024 at 11:35 AM XiDuo You <[email protected]> > wrote: > > > > > Hi folks, > > > > > > It is a discussion about PR-5698[1] and PR-5480[2]. I hope the > > > community can reach an agreement. > > > > > > Recently, we are working on fixing the case-class inheritance issue, > > > thanks to @zhztheplayer, it's great work. However, when touching > > > `VeloxColumnarWriteFilesExec`, we find it introduces higher cost than > > > expected. It requires more code and workaround to resolve the issue of > > > removing case-class inheritance. I think when the cost of solving an > > > issue exceeds the cost of the issue itself, we can consider tolerating > > > the known issue. > > > > > > I propose to revert PR-5480[2] first to avoid regression. > > > > > > Thank you > > > Xiduo > > > > > > [1]: https://github.com/apache/incubator-gluten/pull/5698 > > > [2]: https://github.com/apache/incubator-gluten/pull/5480 > > > > > > --------------------------------------------------------------------- > > > To unsubscribe, e-mail: [email protected] > > > For additional commands, e-mail: [email protected] > > > > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > >
