[RFC] Register scalars with boxed fn impl #9980

jayzhan211 · 2024-04-07T04:20:32Z

Which issue does this PR close?

Closes #9892.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

jayzhan211 · 2024-04-07T04:26:35Z

datafusion/core/src/execution/context/mod.rs

+        if let Some(udf) = functions_array::get_array_udf(name) {
+            return Some(udf);
+        }
+
        self.state.scalar_functions().get(name).cloned()


fallback to user defiend function. Assume they can register actual function . If they need to register closure instead, call array_functions().lock().unwrap().insert(name.to_string(), udf)

jayzhan211 · 2024-04-07T04:29:54Z

datafusion/functions-array/src/lib.rs

+/// Repalce old regsiter_udf
+pub fn register_array_udf(name: &str, udf: ScalarFactory) -> Option<ScalarFactory> {
+    // TODO: Check overwrite?
+    array_functions().lock().unwrap().insert(name.to_string(), udf)


If we switch to static map for storing boxed fn impl, the user can only register with this method

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

jayzhan211 · 2024-04-08T13:41:20Z

@alamb I would like an early comment before moving on.

jayzhan211 · 2024-04-08T13:44:54Z

datafusion/functions-array/src/array_has.rs

@@ -53,6 +53,16 @@ make_udf_function!(ArrayHasAny,
    array_has_any_udf // internal function name
 );

+// TODO: Macro-ify aliases for all functions
+pub fn array_has_aliases() -> Vec<String> {


We can get aliases without instantiating the struct

the downside with this approach

is that now every time this function is called a new Vec of Strings gets made which is likely close to the same cost as instantiating the struct in the first place

This will will be more expensive as if the struct is ever actually called

There are now two Vecs of arrays that need to be kept in sync

Agree with the first 2 points.

There are now two Vecs of arrays that need to be kept in sync

We can replace private aliases.

alamb

Thanks @jayzhan211 -- this looks quite neat

I am quite worried about a mutable global hash table, but otherwise I like where this is heading. We should definitely check with @viirya what his usecase for deferred creation is (aka if we need to change SessionState...)

alamb · 2024-04-08T20:16:09Z

datafusion/functions-array/src/array_has.rs

@@ -53,6 +53,16 @@ make_udf_function!(ArrayHasAny,
    array_has_any_udf // internal function name
 );

+// TODO: Macro-ify aliases for all functions
+pub fn array_has_aliases() -> Vec<String> {


the downside with this approach

is that now every time this function is called a new Vec of Strings gets made which is likely close to the same cost as instantiating the struct in the first place

This will will be more expensive as if the struct is ever actually called

There are now two Vecs of arrays that need to be kept in sync

alamb · 2024-04-08T20:20:47Z

datafusion/functions-array/src/lib.rs

+/// Register a single new UDF, so the user can register their own functions
+///
+/// Repalce old regsiter_udf
+pub fn register_array_udf(name: &str, udf: ScalarFactory) -> Option<ScalarFactory> {


I really don't like that this is some global mutable state that can be changed by the user. I think this will be very confusing as it both may have unintended side effects as well as not being clear if users should register their functions with the SessionState or this global map

I think a much simpler to reason about interface would be that the list of functions in datafusion/functions is immutable. For example something like

pub fn array_functions() -> &'static <HashMap<String, ScalarFactory> {

both may have unintended side effects as well as not being clear if users should register their functions with the SessionState or this global map

My intention is to replace the old one, so there is only a single map that the user need to care about.

alamb · 2024-04-08T20:22:38Z

datafusion/functions-array/src/lib.rs

@@ -139,13 +326,16 @@ pub fn register_all(registry: &mut dyn FunctionRegistry) -> Result<()> {
        replace::array_replace_all_udf(),
        replace::array_replace_udf(),
    ];
+
+    // TODO: Remove. Define in array_functions directly
    functions.into_iter().try_for_each(|udf| {


I think you could avoid the duplication here by looping over the elements of array_functions and instantiating them all

For example (untested)

array_functions() .values() .try_for_each(|udf_factory| { let udf = (udf_factory)(); .. });

We are register functions here, so instantiation should be avoided. Only when we call get_udf is the time we instantiate the function.

Maybe I should use Arc instead of Box to avoid clone

Maybe it is worth clarifying if the goal is to when we are trying to avoi instantiating ScalarUDF

It seems like it could be either:

When we have a physical plan already (aka as in Comet) and need to look up a function by name (but not iterate through the entire list)

Any time a SessionContext is created (aka make FunctionFactory handle deferred registrations too).

I can see the usecase for 1, I am not sure about for 2 🤔

@viirya Do you have suggestions on which usecase is your preferred one?

alamb · 2024-04-08T20:24:20Z

datafusion/functions-array/src/lib.rs

+//
+// Replace register_all with our built-in functions
+// Replace  scalar_functions: HashMap<String, Arc<ScalarUDF>> in SessionState
+pub fn array_functions() -> &'static Mutex<HashMap<String, ScalarFactory>> {


Should we name it to reflect the fact the map is of factories (and not functions)?

Something like

Suggested change

pub fn array_functions() -> &'static Mutex<HashMap<String, ScalarFactory>> {

pub fn array_scalar_factories() -> &'static Mutex<HashMap<String, ScalarFactory>> {

🤔

alamb · 2024-04-08T20:27:14Z

datafusion/core/src/execution/context/mod.rs

+    // Need to implement Clone for this, so probably not a good idea.
+    // scalar_functions_impl: HashMap<String, ScalarFactory>,


It isn't clear to me if @viirya 's usecase needs the defer udf creation in all cases. If the goal is really to avoid instantiting all ScalarUDFs I think a better approach would be to be to chang the signature of FunctionRegistry

However, we may be able to avoid changing session state at all (and simply have an API to get the function factories in addition to registering all functions with the SessionState)

alamb · 2024-04-14T12:30:30Z

Marking as draft as we sort out the requirements

github-actions · 2024-06-14T01:48:53Z

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

first draft

a81494e

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

github-actions bot added the core Core DataFusion crate label Apr 7, 2024

jayzhan211 commented Apr 7, 2024

View reviewed changes

cleanup

0021493

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>

jayzhan211 marked this pull request as ready for review April 8, 2024 13:35

jayzhan211 changed the title ~~Register scalars with boxed fn impl~~ [RFC] Register scalars with boxed fn impl Apr 8, 2024

jayzhan211 commented Apr 8, 2024

View reviewed changes

alamb mentioned this pull request Apr 8, 2024

DataFusion weekly project plan (Andrew Lamb) - April 8, 2024 #10002

Closed

9 tasks

alamb reviewed Apr 8, 2024

View reviewed changes

alamb marked this pull request as draft April 14, 2024 12:30

github-actions bot added the Stale PR has not had any activity for some time label Jun 14, 2024

jayzhan211 closed this Jun 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Register scalars with boxed fn impl #9980

[RFC] Register scalars with boxed fn impl #9980

jayzhan211 commented Apr 7, 2024

jayzhan211 Apr 7, 2024

jayzhan211 Apr 7, 2024

jayzhan211 commented Apr 8, 2024

jayzhan211 Apr 8, 2024 •

edited

Loading

alamb Apr 8, 2024

jayzhan211 Apr 9, 2024

alamb left a comment

alamb Apr 8, 2024

alamb Apr 8, 2024

jayzhan211 Apr 9, 2024

alamb Apr 8, 2024

jayzhan211 Apr 9, 2024 •

edited

Loading

jayzhan211 Apr 9, 2024

alamb Apr 10, 2024

jayzhan211 Apr 11, 2024

alamb Apr 8, 2024

alamb Apr 8, 2024

alamb commented Apr 14, 2024 •

edited

Loading

github-actions bot commented Jun 14, 2024

	pub fn array_functions() -> &'static Mutex<HashMap<String, ScalarFactory>> {
	pub fn array_scalar_factories() -> &'static Mutex<HashMap<String, ScalarFactory>> {

		// Need to implement Clone for this, so probably not a good idea.
		// scalar_functions_impl: HashMap<String, ScalarFactory>,

[RFC] Register scalars with boxed fn impl #9980

[RFC] Register scalars with boxed fn impl #9980

Conversation

jayzhan211 commented Apr 7, 2024

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 commented Apr 8, 2024

jayzhan211 Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayzhan211 Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Apr 14, 2024 • edited Loading

github-actions bot commented Jun 14, 2024

jayzhan211 Apr 8, 2024 •

edited

Loading

jayzhan211 Apr 9, 2024 •

edited

Loading

alamb commented Apr 14, 2024 •

edited

Loading