Yohahaha commented on PR #5743: URL: https://github.com/apache/incubator-gluten/pull/5743#issuecomment-2111446860
> I think there was some initial consideration about storing only general configurations in `cpp/core` as well as `GlutenConfig.h`. > > I do believe we don't yet have a very clear design for relationship between these Scala/CPP modules. But until we start a complete refactor, I think we can just follow the rules we already set so far. > > Regarding the change, I am inclined to not moving all confs to `GlutenConfig.h` (unless we have strong reason), but just moving [these Velox confs](https://github.com/apache/incubator-gluten/blob/37be4aa026c0aa9265a89fb7ae6b4210a1eaafed/cpp/core/config/GlutenConfig.h#L64-L69) from `cpp/core` to `cpp/velox`. All configs are defined in GlutenConfig.scala, include each banckend, but I still need search much place to find related native configs. If I want use/test some behavior with these hidden config, it's inconvenience to move it to open place and change in each PR. This is the reason for me. And there is no reason to separate native configs into different place, since all configs has already been unified in java side. If some design is not solid now, why it can not be changed? I don't treat this PR as a huge refactor, it's the first PR to add tests for memory manager in native side. -- 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]
