feat: Add ConfigOptions to ScalarFunctionArgs #13527
Conversation
github-actions
bot
added
logical-expr
Omega359
changed the title
Omega359
changed the title
|
There is a lot of file changes here but most of the important changes are in scalar_function.rs, There is a todo in expr_simplifier.rs that I would like feedback on. |
|
I plan to review this carefully tomorrow |
There was a problem hiding this comment.
Thank you @Omega359 -- this is an epic plumbing exercise 🪠
The signature in ScalarFunctionArgs is 👌 very nice
This PR seems to require config_options to be cloned many times now. I wonder if it is possible to avoid that 🤔. I took a brief look and it seems to be somewhat challenging as SessionState allows mutable access to the underlying SessionConfig.
Maybe we could change the semantics so that SessionConfig has a Arc<ConfigOptions> which was cloned when it was modified (Arc::unwrap_or_clone() style) 🤔
I also think the const evaluator does need the actual correct ConfigOptions for correctness
| let physical_expr = | ||
| datafusion_physical_expr::create_physical_expr(&expr, &df_schema, &props)?; | ||
| let config_options = Arc::new(ConfigOptions::default()); | ||
| let physical_expr = datafusion_physical_expr::create_physical_expr( |
There was a problem hiding this comment.
It seems somewhat inevitable that creating a physical expr will require the config options
However, I also think threading through the config options down through to the physical creation will (finally) permit people to pass things from the session down to function implementations (I think @cisaacson also was trying to do this in the past)
| @@ -283,10 +284,16 @@ async fn prune_partitions( | |||
|
|
|||
| // TODO: Plumb this down | |||
There was a problem hiding this comment.
This todo may have now be complete
| @@ -336,6 +337,8 @@ pub struct ScalarFunctionArgs<'a> { | |||
| // The return type of the scalar function returned (from `return_type` or `return_type_from_exprs`) | |||
| // when creating the physical expression from the logical expression | |||
| pub return_type: &'a DataType, | |||
| // The config options which can be used to lookup configuration properties | |||
| pub config_options: Arc<ConfigOptions>, | |||
| Ok(e) => e, | ||
| Err(err) => return ConstSimplifyResult::SimplifyRuntimeError(err, expr), | ||
| }; | ||
| // todo - should the config options be the actual options here or is this sufficient? |
There was a problem hiding this comment.
I think the actual configuration options are needed here. Otherwise what will happen is that any function whose behavior relies on the ConfigOptions may have different behavior on columns and constants (or other expressions that can be constant folded)
Yes, it's a bit annoying. I was tempted to see if I could switch to &'a ConfigOptions everywhere. There is at least one 'real' (vs Arc::clone) clone for every query, possibly more as I haven't checked.
Certainly possible, I can attempt that.
I was afraid of that. I was avoiding it because of the signature changes it would required just about everywhere which would cause even more headaches for those systems trying to upgrade. |
|
Yeah, it is a tricky one for sure |
|
Marking as draft as I think this PR is no longer waiting on feedback. Please mark it as ready for review when it is ready for another look |
Which issue does this PR close?
Closes #13519
Rationale for this change
Allow udf's to access df config
What changes are included in this PR?
Code.
Are these changes tested?
Existing tests.
Are there any user-facing changes?
Not specifically, this is covered with the udf signature change in #13290