alamb commented on code in PR #13527:
URL: https://github.com/apache/datafusion/pull/13527#discussion_r1899467353
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -243,6 +292,7 @@ pub fn create_physical_expr(
Arc::new(fun.clone()),
input_phy_exprs.to_vec(),
return_type,
+ Arc::new(config_options.clone()),
Review Comment:
> Pushing the clone higher up the stack is possible but I did run into major
issues trying to push it all the way up including all kinds of disruptive
changes like changing OptimizerConfig from &ConfigOptions to the Arc version.
Doing the opposite - pushing &ConfigOptions all the way down ran into issues
with ScalarFunctionExpr and DynEq/DynHash.
If you have an `Arc<ConfigOptions>` you can always get a `&ConfigOptions` by
calling `options.as_ref()`
So in other words, I don't know if you need to push the Arc through
everywhere (at least at first) -- we could just change the create physical
expr. I would love to help with this -- perhaps I can find time later in the
week (though I have a few other things going on too)
Another option to avoid cloning `ConfigOptions` for each instance of
`ScalarFunction` might be to add some sort of API / way for the function to
communicate "I need the config options"
--
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]
