Create `datafusion-functions-array` crate and move `ArrayToString` function into it #9113

alamb · 2024-02-02T15:02:29Z

Which issue does this PR close?

Part of #8045
See #9100 for discussion

Rationale for this change

We are trying to make DataFusion more modular, and as more array functionality
gets added, it is important to be support:

users choosing which functionality they want to include in their build and
developers to be able to work on the array functions without having to recompile the entire project

What changes are included in this PR?

Make a new datafusion-functions-array crate
Move ArrayToString from BuiltInScalarFunction enum and datafusion-physical-expr to datafusion-functions-array,
Add a new feature flag, array_expressions

If this pattern looks good, I think we can then move the other functions over in a similar way

Are these changes tested?

Yes, by existing and new tests

Are there any user-facing changes?

A new feature flag

Omega359 · 2024-02-02T16:38:05Z

datafusion/functions-array/src/macros.rs

+/// * `DOC`: documentation string for the function
+/// * `SCALAR_UDF_FUNC`: name of the function to create (just) the `ScalarUDF`
+/// * `GNAME`: name for the single static instance of the `ScalarUDF`
+macro_rules! make_udf_function {


Is this going to be copied into every function crate or is this just here for this example migration? Seems perhaps something that could be put into a common/util/etc location

I should have mentioned that -- there is some version of this macro in the datafusion-functions crate - https://github.com/apache/arrow-datafusion/blob/main/datafusion/functions/src/macros.rs

However it isn't quite the same (as the feature flag is on the entire datafusion-functions-array crate rather than on subsets of functions)

My thinking was that we would only have 2 function crates -- datatfusion_functions and datafusion_functions_arrays and thus the replication was ok

We could potentially move the macros into datafusion_expr or datafusion_common perhaps 🤔

alamb · 2024-02-07T15:27:50Z

datafusion/functions-array/src/kernels.rs

+// specific language governing permissions and limitations
+// under the License.
+
+//! implementation kernels for array functions


this code in this module was just moved from array_expressions.rs

comphead · 2024-02-07T16:44:06Z

datafusion/functions-array/src/kernels.rs

+use std::any::type_name;
+use std::sync::Arc;
+
+macro_rules! downcast_arg {


we may want to move into utils,math_expressions.rs has very similar macros, I'll do it in following PR

Thanks @comphead -- once this PR is approved and merged, that would be great

I suspect as we go about porting over the old to new code, we will find other examples that can be cleaned up (some of the functions were implemented very early in DataFusion's life)

comphead · 2024-02-07T16:45:19Z

datafusion/proto/proto/datafusion.proto

@@ -641,7 +641,7 @@ enum ScalarFunction {
  ArrayPrepend = 94;
  ArrayRemove = 95;
  ArrayReplace = 96;
-  ArrayToString = 97;
+  // 97 was ArrayToString


why it was removed?

This should be removed because it is proto for ScalarFunction.
But it seems we need to implement another version for UDF?

This is one of the nice things about making these functions UDF -- they can be serialized / deserialized without any special code.

Here is a test showing it working for encode/decode: https://github.com/apache/arrow-datafusion/blob/8413da8803665c07c2250a0d015f6b67fb7ef743/datafusion/proto/tests/cases/roundtrip_logical_plan.rs#L573

I will add a test case for round tripping array functions too

added in 97a789a

comphead

Thanks @alamb
Very needed initiative
@Weijun-H @jayzhan211 cc

comphead · 2024-02-07T16:49:05Z

Should we include the package/feature in datafusion-cli/Cargo.toml?
Wondering what will happen if user built the cli without array functions feature and tries to invoke array function in SQL

alamb · 2024-02-07T15:28:18Z

datafusion/functions-array/src/udf.rs

+// specific language governing permissions and limitations
+// under the License.
+
+//! [`ScalarUDFImpl`] definitions for array functions.


This is the API defined for the function definitions

alamb · 2024-02-07T15:28:37Z

datafusion/physical-expr/src/array_expressions.rs

@@ -1870,27 +1830,6 @@ pub fn array_replace_all(args: &[ArrayRef]) -> Result<ArrayRef> {
    }
 }

-macro_rules! to_string {


moved to the datafusion-functions-array crate

alamb · 2024-02-07T15:30:03Z

datafusion/proto/src/logical_plan/from_proto.rs

@@ -1462,10 +1460,6 @@ pub fn parse_expr(
                    parse_expr(&args[2], registry)?,
                    parse_expr(&args[3], registry)?,
                )),
-                ScalarFunction::ArrayToString => Ok(array_to_string(


The point of this PR is to remove this enum (and set the pattern on how to do so for the rest of the array functionality

alamb · 2024-02-08T13:49:29Z

datafusion/proto/proto/datafusion.proto

@@ -641,7 +641,7 @@ enum ScalarFunction {
  ArrayPrepend = 94;
  ArrayRemove = 95;
  ArrayReplace = 96;
-  ArrayToString = 97;
+  // 97 was ArrayToString


This is one of the nice things about making these functions UDF -- they can be serialized / deserialized without any special code.

Here is a test showing it working for encode/decode: https://github.com/apache/arrow-datafusion/blob/8413da8803665c07c2250a0d015f6b67fb7ef743/datafusion/proto/tests/cases/roundtrip_logical_plan.rs#L573

I will add a test case for round tripping array functions too

alamb · 2024-02-08T13:53:24Z

Should we include the package/feature in datafusion-cli/Cargo.toml?

Since array_expressions is a default feature, these functions will (still) be in datafusion-cli. I double checked this:

❯ select array_to_string([1,2,3], '---');
+---------------------------------------------------------------------+
| array_to_string(make_array(Int64(1),Int64(2),Int64(3)),Utf8("---")) |
+---------------------------------------------------------------------+
| 1---2---3                                                           |
+---------------------------------------------------------------------+
1 row in set. Query took 0.006 seconds.

Wondering what will happen if user built the cli without array functions feature and tries to invoke array function in SQL

They will get a "no such function" error. It may be worth considering a way to hint / add stub functions that could be registered that say something like "no such function, but there is a function named array_to_string in feature package array_functions" 🤔

comphead

lgtm thanks @alamb
Epic work

github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions optimizer Optimizer rules core Core DataFusion crate documentation Improvements or additions to documentation labels Feb 2, 2024

alamb force-pushed the alamb/array_crate branch from 200508b to cc325e6 Compare February 2, 2024 15:20

This was referenced Feb 2, 2024

Add ColumnarValue::values_to_arrays, deprecate columnar_values_to_array #9114

Merged

[DISCUSS] organization of functions #9100

Closed

Omega359 reviewed Feb 2, 2024

View reviewed changes

This was referenced Feb 4, 2024

DataFusion weekly project plan (Andrew Lamb) - Feb 5, 2024 #9121

Closed

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

Merged

Add datafusion-functions-array crate

1089df9

alamb force-pushed the alamb/array_crate branch from a07d74b to 1089df9 Compare February 7, 2024 15:25

github-actions bot removed the optimizer Optimizer rules label Feb 7, 2024

alamb commented Feb 7, 2024

View reviewed changes

alamb marked this pull request as ready for review February 7, 2024 16:35

comphead reviewed Feb 7, 2024

View reviewed changes

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

8e90d09

alamb commented Feb 8, 2024

View reviewed changes

Add test for round tripping array_to_string

97a789a

alamb mentioned this pull request Feb 9, 2024

Feat/make dfschema wrap schemaref #8905

Closed

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

6ef9aab

comphead approved these changes Feb 11, 2024

View reviewed changes

comphead mentioned this pull request Feb 11, 2024

Add string aggregate grouping fuzz test, add MemTable::with_sort_exprs #9190

Merged

comphead merged commit 292865e into main Feb 12, 2024
44 checks passed

alamb deleted the alamb/array_crate branch February 12, 2024 19:37

alamb mentioned this pull request Feb 20, 2024

[Epic] Port BuiltInFunctons to datafusion-functions-* crates #9285

Closed

43 tasks

jayzhan211 mentioned this pull request Mar 8, 2024

Port StringToArray to function-arrays #9497

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create `datafusion-functions-array` crate and move `ArrayToString` function into it #9113

Create `datafusion-functions-array` crate and move `ArrayToString` function into it #9113

alamb commented Feb 2, 2024 •

edited

Loading

Omega359 Feb 2, 2024

alamb Feb 2, 2024

alamb Feb 7, 2024

comphead Feb 7, 2024

alamb Feb 11, 2024

comphead Feb 7, 2024

jayzhan211 Feb 8, 2024 •

edited

Loading

alamb Feb 8, 2024

alamb Feb 11, 2024

comphead left a comment

comphead commented Feb 7, 2024

alamb Feb 7, 2024

alamb Feb 7, 2024

alamb Feb 7, 2024

alamb Feb 8, 2024

alamb commented Feb 8, 2024

comphead left a comment

Create datafusion-functions-array crate and move ArrayToString function into it #9113

Create datafusion-functions-array crate and move ArrayToString function into it #9113

Conversation

alamb commented Feb 2, 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

jayzhan211 Feb 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

comphead left a comment

Choose a reason for hiding this comment

comphead commented Feb 7, 2024

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 Feb 8, 2024

comphead left a comment

Choose a reason for hiding this comment

Create `datafusion-functions-array` crate and move `ArrayToString` function into it #9113

Create `datafusion-functions-array` crate and move `ArrayToString` function into it #9113

alamb commented Feb 2, 2024 •

edited

Loading

jayzhan211 Feb 8, 2024 •

edited

Loading