RFC: Demonstrate what a function package might look like -- encoding expressions #8046

alamb · 2023-11-03T21:01:04Z

Which issue does this PR close?

Builds on #8039

Demonstrates what #8045 might look like

Rationale for this change

This PR demonstrates what a function package API might look like by removing encoding expressions encode/decode from BuiltInScalarFunction enum and adding it in a separate crate (datafusion-functions)

What changes are included in this PR?

A new FunctionImplementation trait and integration into ScalarUDF to make it easier to write ScalarUDFs;
a new datafusion-functions crate that has the implementation of encode and decode.
Automatically register these functions as part of SessionState::new(), similarly to the automatically registered ListingTables
TODO optional enabling of functions based on feature flag

Open Questions:

to support the expr_fns encode and decode, I think we will need a Expr::ScalarFunction call or something that can take a function by name rather than fully resolved function
Extract registration functions from SessionContext into their own trait / consolidate the function registry code rather than passing
around a set of HahsMaps.... And make a way to actually modify them

Are these changes tested?

Are there any user-facing changes?

datafusion/expr/src/expr_fn.rs

alamb · 2023-11-03T21:42:50Z

datafusion/expr/src/built_in_function.rs

@@ -710,30 +704,6 @@ impl BuiltinScalarFunction {
            BuiltinScalarFunction::Digest => {
                utf8_or_binary_to_binary_type(&input_expr_types[0], "digest")
            }
-            BuiltinScalarFunction::Encode => Ok(match input_expr_types[0] {


This metadata information about the functions is now moved into functions/encoding.rs module, along side its implementation

alamb · 2023-11-03T21:45:15Z

datafusion/expr/src/udf.rs

+    }
+}
+
+/// Convenience trait for implementing ScalarUDF. See [`ScalarUDF::from_impl()`]


This echo's the trait that @2010YOUY01 proposed in ) #7752, but does so in a way that is backwards compatible (makes a ScalarUDF out of the trait, to retain backwards compatibly)

alamb · 2023-11-03T21:46:46Z

datafusion/functions/src/encoding/meta.rs

+
+pub(super) struct EncodeFunc {}
+
+static ENCODE_SIGNATURE: OnceLock<Signature> = OnceLock::new();


Here is what encode and decode look like using the ScalarUDF API -- I think they are much clearer when all this type information is in one place (though I still kept it separate from the implementation to show the implementation did not change at all)

alamb · 2023-11-03T21:47:24Z

datafusion/functions/src/encoding/mod.rs

+use std::collections::HashMap;
+use std::sync::Arc;
+
+/// Registers the `encode` and `decode` functions with the function registry


Here is the conditional registration of these functions based on feature flag -- there are probably nicer ways to do this but I don't think it is any worse than the current solution.

alamb · 2023-11-03T21:48:24Z

datafusion/functions/src/lib.rs

+
+/// Registers all "built in" functions from this crate with the provided registry
+pub fn register_all(registry: &mut HashMap<String, Arc<ScalarUDF>>) {
+    encoding::register(registry);


I envision extending this list with other packages over time.

alamb · 2023-11-04T10:56:11Z

@2010YOUY01 and @viirya I wonder if you have any thoughts on this approach / proposal?

2010YOUY01

Thank you, this looks great. I have several questions/suggestions:

to support the expr_fns encode and decode, I think we will need a Expr::ScalarFunction call or something that can take a function by name rather than fully resolved function

Now constructing an Expr for built-in functions is stateless (does not require context), so it's tricky to be backwards compatible for Expr API, the best solution I can think of is to also support initializing a UDF Expr with only name string, and resolve them during logical plan optimization.

Extract registration functions from SessionContext into their own trait / consolidate the function registry code rather than passing
around a set of HahsMaps.... And make a way to actually modify them

It's a good idea to pack 3 HashMaps for scalar/aggr/window UDFs into a new struct like FunctionRegistry 👍🏼

2010YOUY01 · 2023-11-05T18:52:37Z

datafusion/functions/src/lib.rs

+pub mod utils;
+
+/// Registers all "built in" functions from this crate with the provided registry
+pub fn register_all(registry: &mut HashMap<String, Arc<ScalarUDF>>) {


I think we should support registering a single function here, there might be a use case that the user wants to override only one function from a function package
(possibly by changing this interface to something like

pub fn register_all() { register_package(encoding::all_functions()); register_function(my_encoding::decode()); // override a method in default function package }

2010YOUY01 · 2023-11-05T19:27:42Z

datafusion/expr/src/udf.rs

+    /// Returns this function's name
+    pub fn name(&self) -> &str {
+        &self.name
+    }
+    /// Returns this function's signature
+    pub fn signature(&self) -> &Signature {
+        &self.signature
+    }
+    /// return the return type of this function given the types of the arguments
+    pub fn return_type(&self, args: &[DataType]) -> Result<DataType> {
+        // Old API returns an Arc of the datatype for some reason
+        let res = (self.return_type)(args)?;
+        Ok(res.as_ref().clone())
+    }
+    /// return the implementation of this function
+    pub fn fun(&self) -> &ScalarFunctionImplementation {
+        &self.fun
+    }


Is that the case this set of interfaces is internal-faced for execution, we might extend it during separating function packages?
And trait FunctionImplementation is the user-faced API for defining functions in separate crates

alamb · 2023-11-06T19:06:24Z

Thank you, this looks great. I have several questions/suggestions:

to support the expr_fns encode and decode, I think we will need a Expr::ScalarFunction call or something that can take a function by name rather than fully resolved function

Now constructing an Expr for built-in functions is stateless (does not require context), so it's tricky to be backwards compatible for Expr API, the best solution I can think of is to also support initializing a UDF Expr with only name string, and resolve them during logical plan optimization.

Yes, I agree this approach is the best I can come up with.

Extract registration functions from SessionContext into their own trait / consolidate the function registry code rather than passing
around a set of HahsMaps.... And make a way to actually modify them

It's a good idea to pack 3 HashMaps for scalar/aggr/window UDFs into a new struct like FunctionRegistry 👍🏼

👍 Unfortunately that name is already taken :) Maybe MemoryFunctionRegistry 🤔

…g_expressions

alamb · 2023-11-19T22:18:50Z

datafusion/execution/src/registry.rs

@@ -34,6 +34,17 @@ pub trait FunctionRegistry {

    /// Returns a reference to the udwf named `name`.
    fn udwf(&self, name: &str) -> Result<Arc<WindowUDF>>;
+
+    /// Registers a new `ScalarUDF`, returning any previously registered


this is a new proposed API -- to allow registering new scalar UDFs with a FunctionRegistry.

alamb · 2023-11-19T22:26:44Z

datafusion/expr/src/expr_fn.rs

@@ -1228,30 +1229,4 @@ mod test {
            unreachable!();
        }
    }
-
-    #[test]
-    fn encode_function_definitions() {


I don't think these tests add a lot -- they simply encode the signature again. This is also covered by actually calling encode() via the expr API / dataframe tests which is done.

…g_expressions

alamb · 2023-12-15T22:09:20Z

datafusion/functions/src/encoding/mod.rs

+pub mod expr_fn {
+    use super::*;
+    /// Return encode(arg)
+    pub fn encode(args: Vec<Expr>) -> Expr {


here are the new expr_fn implementations

…g_expressions

alamb added 4 commits November 3, 2023 14:19

Make fields of ScalarUDF non pub

c67e620

Add datafusion-functions crate

01a9bce

Extract registration functions into a trait

ee7a873

Complete integration

4b5acd1

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core datafusion crate labels Nov 3, 2023

Conditional compilation

c441a0d

alamb force-pushed the alamb/extract_encoding_expressions branch from b3e25be to c441a0d Compare November 3, 2023 21:42

alamb commented Nov 3, 2023

View reviewed changes

alamb added 3 commits November 3, 2023 17:56

Update lock

d38a80a

put back deleted part and add RAT

1841436

tomlfmt and doc

7c7065e

alamb mentioned this pull request Nov 4, 2023

[EPIC] Unify Function Interface (remove BuiltInScalarFunction) #8045

Closed

22 tasks

2010YOUY01 reviewed Nov 5, 2023

View reviewed changes

alamb mentioned this pull request Nov 13, 2023

Support User Defined Table Functions / Table Value Functions #7926

Closed

Merge remote-tracking branch 'apache/main' into alamb/extract_encodin…

4370bdd

…g_expressions

github-actions bot removed the optimizer Optimizer rules label Nov 18, 2023

Implement new function registry interface

b5d4b4f

alamb commented Nov 19, 2023

View reviewed changes

alamb added 4 commits November 28, 2023 15:24

Merge remote-tracking branch 'apache/main' into alamb/extract_encodin…

094a0e2

…g_expressions

update commit

02ccb3a

remove dead code

be159d1

Update readme

9871ca4

This was referenced Dec 5, 2023

DataFusion weekly project plan (Andrew Lamb) - Dec 4, 2023 #8420

Closed

DataFusion weekly project plan (Andrew Lamb) - Dec 11, 2023 #8490

Closed

alamb added 4 commits December 15, 2023 15:17

Update

a9f4f92

Add proto test

fbf7d44

Implement expr_fn

a95917e

simplify

dabd5b8

alamb commented Dec 15, 2023

View reviewed changes

alamb mentioned this pull request Dec 15, 2023

Support Expr creation for ScalarUDF: Resolve function calls by name during planning #8157

Closed

alamb added 6 commits December 15, 2023 17:21

make it compile

46ea79f

macro magic

6c6a742

put macros in their own file

fe397c1

Merge remote-tracking branch 'apache/main' into alamb/extract_encodin…

a91e7d2

…g_expressions

cleanup

ea14fe9

Merge remote-tracking branch 'apache/main' into alamb/extract_encodin…

817382f

…g_expressions

alamb mentioned this pull request Dec 18, 2023

DataFusion weekly project plan (Andrew Lamb) - Dec 18, 2023 #8577

Closed

7 tasks

alamb mentioned this pull request Jan 1, 2024

Create datafusion-functions crate, extract encode and decode to #8705

Merged

alamb closed this in #8705 Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Demonstrate what a function package might look like -- encoding expressions #8046

RFC: Demonstrate what a function package might look like -- encoding expressions #8046

alamb commented Nov 3, 2023 •

edited

Loading

alamb Nov 3, 2023 •

edited

Loading

alamb Nov 3, 2023

alamb Nov 3, 2023

alamb Nov 3, 2023

alamb Nov 3, 2023

alamb commented Nov 4, 2023

2010YOUY01 left a comment

2010YOUY01 Nov 5, 2023

2010YOUY01 Nov 5, 2023

alamb commented Nov 6, 2023

alamb Nov 19, 2023

alamb Nov 19, 2023 •

edited

Loading

alamb Dec 15, 2023


		pub(super) struct EncodeFunc {}

		static ENCODE_SIGNATURE: OnceLock<Signature> = OnceLock::new();

RFC: Demonstrate what a function package might look like -- encoding expressions #8046

RFC: Demonstrate what a function package might look like -- encoding expressions #8046

Conversation

alamb commented Nov 3, 2023 • edited Loading

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?

alamb Nov 3, 2023 • 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

alamb commented Nov 4, 2023

2010YOUY01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Nov 6, 2023

Choose a reason for hiding this comment

alamb Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Nov 3, 2023 •

edited

Loading

alamb Nov 3, 2023 •

edited

Loading

alamb Nov 19, 2023 •

edited

Loading