alamb commented on PR #16573: URL: https://github.com/apache/datafusion/pull/16573#issuecomment-3029236099
> It looks we didn't reach a conclusion here yet. There is "just a String" option as currently implemented in this PR. There is "a new struct passed by reference (or Arc)" option as proposed in the comment above. There is also "add SessionConfig by reference (or Arc)" option (#13519) that I am not very supportive of. > > @alamb @Omega359 thoughts? So it is still my opinion that copying fields from `ConfigProperties` into` ExecutionProps` one at a time will over time result in more and more "hair" (aka we'll have some subset of the fields and it won't be clear why some are included and some are not) For that reason, I still think figuring out some way to pass `ConfigOption`s via ExecutionProps is the right thing to do long term. Also as @Omega359 using ConfigOptions aligns us with the asyn udfs as well. I was wondering if we can change`SessionState` to have an `Arc<ConfigOptions>` and pass that into ExecutionProperties We could use `Arc::make_mut` to implement "copy on write" semantics. Let me see if I can whip up a demo -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
