feat: Add `Arc<ConfigOptions>` to `ScalarFunctionArgs`, don't copy `ConfigOptions` on each query #16970

Omega359 · 2025-07-29T18:07:05Z

Which issue does this PR close?

Closes

Add SessionConfig reference to ScalarFunctionArgs #13519

This is a continuation of @alamb's PR

closes POC: Add ConfigOptions to ExecutionProps when execution is started #16661

Relates to:

Alternative to:

Rationale for this change

Allow udf's to access df config to allow for their behaviour to change based on configuration. For example, allows date and timestamp udf's to use a different timezone than UTC or to allow date/timestamp parsing to have ANSI behaviour when parsing fails.

What changes are included in this PR?

Add an Arc<ConfigOptions> to ExecutionProps when the execution is started
SessionState::config_options() now returns &Arc<ConfigOptions> instead of &ConfigOptions
Add ConfigOptions to ScalarFunctionArgs
AsyncScalarUdf.invoke_async_with_args(..) removed the config_options arg
OptimizerConfig.options() return value changed from &ConfigOptions to Arc<ConfigOptions>']
a bunch of fn's in the proto code had the argument registry: &dyn FunctionRegistry changed to ctx: &SessionContext
new test

Note that there is two todo's in ffi/src/udf/mod.rs related to the serdes of config options in ScalarFunctionArgs. This PR doesn't do any serdes for that.

Are these changes tested?

Existing tests + a new test 'test_config_options_work_for_scalar_func' in user_defined_scalar_functions.rs

Are there any user-facing changes?

Yes, breaking changes as listed above.

…ke_async_with_args to remove ConfigOptions arg.

…clone() calls. Fixed an issue with SimplifyExpressions.rewrite(..) not adding config options to execution_props. Added test to verify it works

comphead

Thanks @Omega359 that looks very helpful and addresses the important problem that builtin functions could not be parametrized by DF runtime properties. I'm planning to review it today

comphead

I tested this PR on couple of functions and runtime config passes properly to the internal invocation function.

However what comes to my mind is passing runtime properties to built-in SQL functions can be powerful but introduces several trade-offs worth considering.

Good:

It allows function behavior to be dynamically adapted based on the execution context (e.g., user locale, timezone, session settings, environment specifics).
Especially this would be increasingly helpful for downstream projects which have to copy DF function with minor changes and maintain that the code is in sync

Not so good:
DF can end up with bloating code serving the purpose of each downstream project.

Apparently reviewers need to verify this mechanism is used properly

datafusion/core/src/execution/session_state.rs

comphead · 2025-07-31T16:08:24Z

datafusion/optimizer/src/optimizer.rs

        }
    }

    /// Specify whether to enable the filter_null_keys rule
    pub fn filter_null_keys(mut self, filter_null_keys: bool) -> Self {
-        self.options.optimizer.filter_null_join_keys = filter_null_keys;
+        Arc::make_mut(&mut self.options)


using make_mut can cause extra inner cloning if other Arcs already points to the object https://doc.rust-lang.org/std/sync/struct.Arc.html#method.make_mut

Yeah -- I think this is unavoidable if someone wants to modify the config options

I think it is better than what is on main as today every OptimizerConfig requires a clone of the ConfigOptions -- using make_mut means the code will now only clone the config when it actually needs to be modified

alamb

Thank you @Omega359

I have a few API suggestions. I sketched the major one out here

Omega359#6

If we go with that PR I'll add another note to the upgrade guide

I also pushed a commit about the change to async UDFs to the upgrade guide

datafusion/core/src/execution/session_state.rs

alamb · 2025-07-31T21:06:00Z

datafusion/core/tests/user_defined/user_defined_scalar_functions.rs

+    let expected_schema = Schema::new(vec![Field::new("a", DataType::Utf8, false)]);
+    let expected = RecordBatch::try_new(
+        SchemaRef::from(expected_schema),
+        vec![create_array!(Utf8, ["AEST"])],


datafusion/execution/src/config.rs

datafusion/expr/src/execution_props.rs

alamb · 2025-07-31T21:26:03Z

FYI @findepi (who I believe is away this week but may be interested in this PR)

Co-authored-by: Andrew Lamb <[email protected]>

Omega359 · 2025-08-01T21:59:16Z

Good:

It allows function behavior to be dynamically adapted based on the execution context (e.g., user locale, timezone, session settings, environment specifics). Especially this would be increasingly helpful for downstream projects which have to copy DF function with minor changes and maintain that the code is in sync

Not so good: DF can end up with bloating code serving the purpose of each downstream project.

Apparently reviewers need to verify this mechanism is used properly

Indeed. Beyond runtime switching of behaviour for custom UDF's I foresee two main use cases for the core udf's - timezone and spark-like 'ansi' mode.

alamb · 2025-08-04T16:51:32Z

Not so good: DF can end up with bloating code serving the purpose of each downstream project.
Apparently reviewers need to verify this mechanism is used properly

Indeed. Beyond runtime switching of behaviour for custom UDF's I foresee two main use cases for the core udf's - timezone and spark-like 'ansi' mode.

While I agree that this PR makes it easier to add more functionality (and thus bloat) to the core functions, I don't think it fundamentally changes the need for reviewers to help make that tradeoff when evaluating new features for inclusion

comphead · 2025-08-04T16:54:07Z

Are we good to merge the PR?

alamb · 2025-08-04T16:57:37Z

datafusion/expr/src/execution_props.rs

@@ -68,9 +72,10 @@ impl ExecutionProps {

    /// Marks the execution of query started timestamp.
    /// This also instantiates a new alias generator.
-    pub fn start_execution(&mut self) -> &Self {
+    pub fn mark_start_execution(&mut self, config_options: Arc<ConfigOptions>) -> &Self {


i think this is technically a breaking API. It would be nicer for downstream users if we kept the old function and deprecated it so the compiler told them what was happening.

I pushed a commit to do that

alamb · 2025-08-04T17:08:23Z

datafusion/optimizer/src/optimizer.rs

        }
    }

    /// Specify whether to enable the filter_null_keys rule
    pub fn filter_null_keys(mut self, filter_null_keys: bool) -> Self {
-        self.options.optimizer.filter_null_join_keys = filter_null_keys;
+        Arc::make_mut(&mut self.options)


Yeah -- I think this is unavoidable if someone wants to modify the config options

I think it is better than what is on main as today every OptimizerConfig requires a clone of the ConfigOptions -- using make_mut means the code will now only clone the config when it actually needs to be modified

datafusion/ffi/src/udf/mod.rs

alamb · 2025-08-04T17:21:39Z

I touched up some more docs, added another note to the upgrade guide, and filed a ticket for the TODO items

I think this PR is ready to go and I plan to merge it tomorrow unless anyone else would like additional time to comment or review

alamb · 2025-08-04T17:29:39Z

Are we good to merge the PR?

Sorry I was working on a few last touchups - I think we are good to merge it now, but was going to leave it open for one more day to gather any potential additional feedback. Perhaps this is not necessary

alamb · 2025-08-05T16:10:01Z

🚀

alamb · 2025-08-05T16:36:39Z

Thanks again @Omega359

alamb · 2025-08-08T15:16:53Z

datafusion/physical-expr/src/scalar_function.rs

+                    .config_options
+                    .entries()
+                    .iter()
+                    .sorted_by(|&l, &r| l.key.cmp(&r.key))


I found this code while reviewing #17078

It seems to me that adding this sort of all config options to each scalar function when looking for equality is going to be quite expensive at planning time.

Can we potentially do a pointer comparison first before deciding we need to do a deep compare by value?

Within eq impl it's easy to do pointer comparison first, but what about Hash impl?
Is the assumption that eq is called during planning much more often than the hash?

Within eq impl it's easy to do pointer comparison first, but what about Hash impl? Is the assumption that eq is called during planning much more often than the hash?

I think for the hash implementation we can just not hash the config_options -- while this might result in theoretically more hash collisions it will be way faster and still correct

I think for the hash implementation we can just not hash the config_options --

sgtm

Improve Hash speed for ScalarFunctionExpr #17099

…onfigOptions` on each query (apache#16970) * Add `ConfigOptions` to ExecutionProps when execution is started * Add ConfigOptions to ScalarFunctionArgs, refactor AsyncScalarUDF.invoke_async_with_args to remove ConfigOptions arg. * Updated OptimizerConfig.options() -> Arc<ConfigOptions> to eliminate clone() calls. Fixed an issue with SimplifyExpressions.rewrite(..) not adding config options to execution_props. Added test to verify it works * Test update. * Add note in upgrade guide * Use Arc all the way down * start_execution -> mark_start_execution * Update datafusion/expr/src/execution_props.rs Co-authored-by: Andrew Lamb <[email protected]> * Update comments * Avoid API breakage via #deprecated * Update upgrade guide for Arc<ConfigOptions> change * Apply suggestions from code review * fmt --------- Co-authored-by: Andrew Lamb <[email protected]>

alamb and others added 3 commits July 29, 2025 13:41

Add ConfigOptions to ExecutionProps when execution is started

27289fe

Add ConfigOptions to ScalarFunctionArgs, refactor AsyncScalarUDF.invo…

2e0ffcd

…ke_async_with_args to remove ConfigOptions arg.

Updated OptimizerConfig.options() -> Arc<ConfigOptions> to eliminate …

383e5a8

…clone() calls. Fixed an issue with SimplifyExpressions.rewrite(..) not adding config options to execution_props. Added test to verify it works

Omega359 mentioned this pull request Jul 29, 2025

Wire up ConfigOptions alamb/datafusion#34

Closed

Omega359 added 2 commits July 29, 2025 18:34

Merge remote-tracking branch 'upstream/main' into udf_config_options

5bad5f7

Test update.

7524758

Omega359 changed the title ~~Add ConfigOptions to ScalarFunctionArgs~~ feat: Add ConfigOptions to ScalarFunctionArgs Jul 29, 2025

Omega359 mentioned this pull request Jul 30, 2025

[DISCUSSION] Conditional Utf8View support for downstream projects #16903

Open

comphead reviewed Jul 31, 2025

View reviewed changes

comphead approved these changes Jul 31, 2025

View reviewed changes

comphead reviewed Jul 31, 2025

View reviewed changes

datafusion/core/src/execution/session_state.rs Show resolved Hide resolved

comphead reviewed Jul 31, 2025

View reviewed changes

datafusion/core/src/execution/session_state.rs Outdated Show resolved Hide resolved

comphead reviewed Jul 31, 2025

View reviewed changes

alamb added 2 commits July 31, 2025 17:10

Add note in upgrade guide

55d3b73

Use Arc all the way down

92af2d7

alamb mentioned this pull request Jul 31, 2025

Use Arc all the way down Omega359/arrow-datafusion#6

Merged

alamb approved these changes Jul 31, 2025

View reviewed changes

Omega359 and others added 2 commits August 1, 2025 13:39

start_execution -> mark_start_execution

14c90ed

Update datafusion/expr/src/execution_props.rs

d7d2247

Co-authored-by: Andrew Lamb <[email protected]>

Omega359 mentioned this pull request Aug 2, 2025

feat: Add ConfigOptions to ScalarFunctionArgs #13527

Closed

Merge remote-tracking branch 'apache/main' into udf_config_options

7758028

alamb changed the title ~~feat: Add ConfigOptions to ScalarFunctionArgs~~ feat: Add Arc<ConfigOptions> to ScalarFunctionArgs, Aug 4, 2025

alamb added 3 commits August 4, 2025 12:58

Update comments

6470b46

Avoid API breakage via #deprecated

436256e

Update upgrade guide for Arc<ConfigOptions> change

f3fd77b

alamb changed the title ~~feat: Add Arc<ConfigOptions> to ScalarFunctionArgs,~~ feat: Add Arc<ConfigOptions> to ScalarFunctionArgs, don't copy ConfigOptions on each query Aug 4, 2025

alamb approved these changes Aug 4, 2025

View reviewed changes

alamb mentioned this pull request Aug 4, 2025

Support passing ConfigOptions to scalar functions via FFI #17035

Open

alamb reviewed Aug 4, 2025

View reviewed changes

datafusion/ffi/src/udf/mod.rs Outdated Show resolved Hide resolved

datafusion/ffi/src/udf/mod.rs Outdated Show resolved Hide resolved

alamb added 2 commits August 4, 2025 13:20

Apply suggestions from code review

238a7ed

fmt

d6588db

alamb added 2 commits August 4, 2025 14:25

Merge remote-tracking branch 'apache/main' into udf_config_options

0442fa8

Merge branch 'main' into udf_config_options

ffbb331

alamb merged commit e4f16dd into apache:main Aug 5, 2025
28 checks passed

alamb mentioned this pull request Aug 5, 2025

Add SessionConfig reference to ScalarFunctionArgs #13519

Closed

This was referenced Aug 5, 2025

Make from_unixtime aware of execution timezone #12892

Open

datafusion.execution.time_zone is not used for basic time zone inference #13212

Open

alamb reviewed Aug 8, 2025

View reviewed changes

alamb mentioned this pull request Aug 8, 2025

Fix hash/equality issues for ScalarFunctionExpr #17078

Merged

feat: Add Arc<ConfigOptions> to ScalarFunctionArgs, don't copy ConfigOptions on each query #16970

feat: Add Arc<ConfigOptions> to ScalarFunctionArgs, don't copy ConfigOptions on each query #16970

Uh oh!

Conversation

Omega359 commented Jul 29, 2025 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alamb commented Jul 31, 2025

Uh oh!

Omega359 commented Aug 1, 2025

Uh oh!

alamb commented Aug 4, 2025

Uh oh!

comphead commented Aug 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alamb commented Aug 4, 2025

Uh oh!

alamb commented Aug 4, 2025

Uh oh!

alamb commented Aug 5, 2025

Uh oh!

Uh oh!

alamb commented Aug 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feat: Add `Arc<ConfigOptions>` to `ScalarFunctionArgs`, don't copy `ConfigOptions` on each query #16970

feat: Add `Arc<ConfigOptions>` to `ScalarFunctionArgs`, don't copy `ConfigOptions` on each query #16970

Omega359 commented Jul 29, 2025 •

edited by alamb

Loading

alamb Aug 8, 2025 •

edited

Loading