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

alamb · 2024-01-01T13:55:52Z

Note: I view this PR as strategically very important because:

It proposes a pattern to make the DataFusion core more modular
One we set the pattern, I think the community will likely help do the work (as they have done in Port remaining sql integration tests from rust to sqllogictest #6195)

While this PR looks large, it is a lot of boiler plate and comments. The actual changes are quite small (move two functions)

Which issue does this PR close?

Part of #8045

Closes #8046
Closes #8157 (it is not needed anymore)

Rationale for this change

This is the start of removing the distinction between "Built in" functions and "User Defined Functions"

Removing the distinction between "Built in" functions and "User Defined Functions" has two benefits:

It ensures that User Defined Functions have the exact same functionality as "built in" functions
It allows better customization of DataFusion (e.g. for people who want "postgres like" functionality vs people who want "spark like" functionality) by making it easier to mix different packages.

What changes are included in this PR?

Add a new datafusion-functions crate (eventually I imagine there may be several subcrates, to allow faster compilation, but let's start with one)
Adds a new API convention for a function "package" and examples for how to write
Add a register_udf function to the FunctionRegistry trait
Extract the encode and decode functions into the new crate
Tests showing that protobuf serialization works for these functions

Are these changes tested?

Yes, there are new unit tests

Are there any user-facing changes?

The definition of the expr fn API for encode and decode are now defined in datafusion-functions rather than datafusion-expr. For anyone who just uses the expr_fn APIs this will mean a new dependency on datafusion-physical-expr.

alamb · 2024-01-02T19:06:31Z

datafusion/expr/src/built_in_function.rs

@@ -69,14 +69,10 @@ pub enum BuiltinScalarFunction {
    Cos,
    /// cos
    Cosh,
-    /// Decode


This is the key point of this PR: to begin removing items from this (giant) enum

alamb · 2024-01-02T19:06:51Z

datafusion/expr/src/built_in_function.rs

@@ -758,30 +752,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 logic is moved into the ScalarUDFImpl for Encode/Decod

alamb · 2024-01-02T19:29:23Z

datafusion/expr/src/expr_fn.rs

@@ -1310,34 +1306,4 @@ mod test {
            unreachable!();
        }
    }
-
-    #[test]


These tests are testing the expression construction which I don't think is hugely valuable -- what is far more important is that encode/decode can be called as SQL and the dataframe API

encode/decode is covered by in slt here: https://github.com/apache/arrow-datafusion/blob/d68c0fcb491d4d0a2159ff433b59fa77f78edca4/datafusion/sqllogictest/test_files/encoding.slt#L18-L17

And I have added an encode/decode dataframe test in this PR

alamb · 2024-01-02T19:49:25Z

datafusion/proto/tests/cases/roundtrip_logical_plan.rs

@@ -524,6 +527,30 @@ async fn roundtrip_logical_plan_with_extension() -> Result<()> {
    Ok(())
 }

+#[tokio::test]
+async fn roundtrip_expr_api() -> Result<()> {


Here are tests demonstrating that using ScalarUDF to implement functions are properly serialized

alamb · 2024-01-15T14:11:45Z

datafusion/core/tests/dataframe/dataframe_functions.rs

@@ -750,3 +756,48 @@ async fn test_fn_upper() -> Result<()> {

    Ok(())
 }
+
+#[tokio::test]
+async fn test_fn_encode() -> Result<()> {


these functions weren't previously tested. I added tests to show that it is still possible to use the fluent API even with the moving of the expr_fn implementation

alamb · 2024-01-15T14:11:58Z

datafusion/execution/src/registry.rs

+        not_impl_err!("Registering ScalarUDF")
+    }
+
+    // TODO add register_udaf and register_udwf


I will file follow on tickets for this

alamb · 2024-01-15T14:12:34Z

datafusion/execution/src/registry.rs

+
+/// A  [`FunctionRegistry`] that uses in memory [`HashMap`]s
+#[derive(Default, Debug)]
+pub struct MemoryFunctionRegistry {


This is currently only used for testing, but I plan to reduce the duplication between SessionContext, and TaskContext using it

alamb · 2024-01-15T14:13:14Z

datafusion/functions/README.md

+  under the License.
+-->
+
+# DataFusion Function Library


Here is the basic idea -- make a new crate with all the function definitions

alamb · 2024-01-15T14:14:38Z

datafusion/physical-expr/src/functions.rs

-        }
-    };
-}
-
 #[cfg(feature = "crypto_expressions")]


The other existing feature flags such as crypto_expressions are obvious candidates to extract into datafusion-functions

alamb · 2024-01-15T14:15:54Z

I think we should wait to merge this until after releasing #8863

ozankabak · 2024-01-23T14:52:38Z

@metesynnada PTAL

mustafasrepo

LGTM!. I think this PR is great in accomplishing your aim:

This is the start of removing the distinction between "Built in" functions and "User Defined Functions"

This will enable force us to use, create good APIs for external users. Since now every scalar function is like User Defined function. Thanks @alamb for this PR.
I think, in the future your intent is to move all functions in the BuiltinScalarFunction enum to this package. If this is the intent, maybe we can add a recipe somewhere (can be a issue body) for explaining steps to convert existing BuiltinScalarFunctions to ScalarUDFImpls

crepererum

alamb · 2024-01-29T17:50:24Z

Thank you @mustafasrepo @metesynnada and @crepererum for your reviews. I plan to merge this PR tomorrow unless anyone else would like extra time to review, and file follow on issues to start organizing the bigger project

sunchao

LGTM. This looks like a pretty nice feature!

viirya · 2024-01-29T19:27:52Z

datafusion/functions/src/encoding/inner.rs

+    }
+
+    fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
+        // Put a feature flag here to make sure this is only compiled when the feature is activated


Do we need a TODO here?

This is a good call -- I think the comment is out of date -- this whole module is only compiled when the feature is activated. I will remove it

datafusion/functions/src/lib.rs

viirya · 2024-01-29T19:39:36Z

datafusion/functions/src/macros.rs

+macro_rules! make_package {
+    ($name:ident, $feature:literal, $DOC:expr) => {
+        #[cfg(feature = $feature)]


Hmm, I wonder if all function packages have corresponding features? I guess for built-in scalar functions they don't have feature?

This is an excellent observation

I think eventually it would be nice to have as many functions as possible be optional and behind a feature flat. There are some functions that back built in Expr like make_array

What I am thinking is that we'll first break out the functions that do have their own feature flags already and establish a pattern and then we can design / lift out others that don't. I hope to write down more of the plan shortly

Here is a discussion / proposal on how the functions could be organized: #9100

Omega359 · 2024-01-29T19:50:46Z

Looks nice, I think this should really help with the readability of the code

Co-authored-by: Liang-Chi Hsieh <[email protected]>

…fusion into alamb/function_packages

alamb · 2024-01-30T22:49:55Z

🚀

alamb · 2024-01-31T14:18:41Z

Thank you everyone who helped review and provide commentary on this. @viirya @sunchao @2010YOUY01 @crepererum @metesynnada and @mustafasrepo -- this is only the first step but I am pretty excited about where this will go

Omega359 · 2024-02-07T18:46:29Z

A couple of questions on this:

Where should public rustdocs for the function go?
Benchmarks? Tests && test data? Is that all going to live at functions/benches and functions/tests ?

alamb · 2024-02-08T11:09:17Z

I responded to these questions above at #9100 (comment)

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions core Core DataFusion crate labels Jan 1, 2024

alamb mentioned this pull request Jan 1, 2024

Add trait based ScalarUDF API #8578

Merged

2 tasks

alamb force-pushed the alamb/function_packages branch from d9528ab to d9378a9 Compare January 2, 2024 19:03

alamb commented Jan 2, 2024

View reviewed changes

alamb force-pushed the alamb/function_packages branch from d9378a9 to 7f860dc Compare January 2, 2024 19:29

Extract encode and decode to datafusion-functions crate

f2846b0

alamb commented Jan 2, 2024

View reviewed changes

better docs

1a1e0f4

alamb force-pushed the alamb/function_packages branch from 7f860dc to 1a1e0f4 Compare January 3, 2024 14:38

alamb added 4 commits January 15, 2024 08:33

Merge remote-tracking branch 'apache/main' into alamb/function_packages

304ce2d

Improve docs + macros

259f122

tweaks

68b9289

updates

5deddb0

alamb commented Jan 15, 2024

View reviewed changes

alamb marked this pull request as ready for review January 15, 2024 14:15

alamb changed the title ~~Extract encode and decode to datafusion-functions crate~~ Create datafusion-functions crate, extract encode and decode to Jan 15, 2024

alamb added 5 commits January 15, 2024 10:05

fix doc

f75472c

tomlfmt

3d3d03a

fix doc

6f961be

Merge remote-tracking branch 'apache/main' into alamb/function_packages

a959996

Merge remote-tracking branch 'apache/main' into alamb/function_packages

1262f52

alamb mentioned this pull request Jan 21, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 22, 2024 #8933

Closed

9 tasks

alamb added 2 commits January 23, 2024 07:03

Merge remote-tracking branch 'apache/main' into alamb/function_packages

841e32a

update datafusion-cli Cargo.locl

2d21c35

This was referenced Jan 24, 2024

Support computing ScalarValue return types from argument values (e.g. constants) rather than just types #8624

Closed

Initial PR apache/datafusion-comet#1

Merged

alamb added 2 commits January 27, 2024 04:14

Merge remote-tracking branch 'apache/main' into alamb/function_packages

5881756

update datafusion-cli cargo.lock

cc6268e

alamb requested review from viirya, crepererum, mustafasrepo and metesynnada January 28, 2024 12:25

alamb mentioned this pull request Jan 28, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 29, 2024 #9030

Closed

6 tasks

mustafasrepo approved these changes Jan 29, 2024

View reviewed changes

metesynnada approved these changes Jan 29, 2024

View reviewed changes

crepererum approved these changes Jan 29, 2024

View reviewed changes

sunchao approved these changes Jan 29, 2024

View reviewed changes

viirya reviewed Jan 29, 2024

View reviewed changes

datafusion/functions/src/lib.rs Outdated Show resolved Hide resolved

viirya reviewed Jan 29, 2024

View reviewed changes

viirya approved these changes Jan 29, 2024

View reviewed changes

alamb and others added 5 commits January 29, 2024 17:28

Merge remote-tracking branch 'apache/main' into alamb/function_packages

76b1650

Remove outdated comment, make non pub

f3c311d

Apply suggestions from code review

159cfbd

Co-authored-by: Liang-Chi Hsieh <[email protected]>

Merge branch 'alamb/function_packages' of github.com:alamb/arrow-data…

a15178e

…fusion into alamb/function_packages

Merge remote-tracking branch 'apache/main' into alamb/function_packages

af79f35

alamb mentioned this pull request Jan 30, 2024

Add FunctionRegistry::register_udaf and FunctionRegistry::register_udwf #9075

Merged

alamb merged commit d6d35f7 into apache:main Jan 30, 2024
22 checks passed

alamb deleted the alamb/function_packages branch January 31, 2024 14:18

alamb mentioned this pull request Feb 1, 2024

[DISCUSS] organization of functions #9100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alamb commented Jan 1, 2024 •

edited

Loading

alamb Jan 2, 2024

alamb Jan 2, 2024

alamb Jan 2, 2024

alamb Jan 2, 2024

alamb Jan 15, 2024

alamb Jan 15, 2024

alamb Jan 30, 2024

alamb Jan 15, 2024

alamb Jan 15, 2024

alamb Jan 15, 2024

alamb commented Jan 15, 2024

ozankabak commented Jan 23, 2024

mustafasrepo left a comment •

edited

Loading

crepererum left a comment

alamb commented Jan 29, 2024

sunchao left a comment

viirya Jan 29, 2024

alamb Jan 29, 2024

viirya Jan 29, 2024

alamb Jan 30, 2024

alamb Feb 1, 2024

Omega359 commented Jan 29, 2024

alamb commented Jan 30, 2024

alamb commented Jan 31, 2024

Omega359 commented Feb 7, 2024

alamb commented Feb 8, 2024

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

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

Conversation

alamb commented Jan 1, 2024 • 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?

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

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 Jan 15, 2024

ozankabak commented Jan 23, 2024

mustafasrepo left a comment • edited Loading

Choose a reason for hiding this comment

crepererum left a comment

Choose a reason for hiding this comment

alamb commented Jan 29, 2024

sunchao 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

Choose a reason for hiding this comment

Omega359 commented Jan 29, 2024

alamb commented Jan 30, 2024

alamb commented Jan 31, 2024

Omega359 commented Feb 7, 2024

alamb commented Feb 8, 2024

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

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

alamb commented Jan 1, 2024 •

edited

Loading

mustafasrepo left a comment •

edited

Loading