move make_array array_append array_prepend array_concat function to datafusion-functions-array crate #9504

guojidan · 2024-03-08T10:05:36Z

Which issue does this PR close?

Closes #9322.

Rationale for this change

What changes are included in this PR?

move make_array array_append array_prepend array_concat function to datafusion-functions-array crate

Are these changes tested?

Are there any user-facing changes?

guojidan · 2024-03-08T10:08:21Z

because #9343 pr lasting for too long，difficult to rebase or merge, so I open a new pr

jayzhan211

I take a quick glance and I think it is ready to merge.

There are 2 things that I'm trying to figure out to improve further

Make sure we have Scalar that we need make_scalar_function_with_hints, since I think we should only get Array for array functions, if we really have Scalar, it seems something wrong to me.
Simplify cfg feature flag, I would like to change OperatorToFunction to OperatorToArrayFunciton and see whether we can import the entire rule (file) optionally.

They don't need to be solved in this PR, but I think they are possible to be improved

jayzhan211 · 2024-03-08T12:48:23Z

datafusion/optimizer/Cargo.toml

 chrono = { workspace = true }
 datafusion-common = { workspace = true, default-features = true }
 datafusion-expr = { workspace = true }
+datafusion-functions-array = { workspace = true, optional = true }


@alamb I think having optional importing is fine. Is there any other downside to this?

jayzhan211 · 2024-03-09T01:33:20Z

let me rebase and fix the conflicts

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-03-09T03:40:15Z

Make sure we have Scalar that we need make_scalar_function_with_hints, since I think we should only get Array for array functions, if we really have Scalar, it seems something wrong to me.

I think we need make_scalar_function_with_hints, Scalar::List are represented in ColumnValue::Scalar, and others are represented in ColumnValue::Array. Although we don't do the optimized Scalar path for array functions, to be consistent with the definition of ColumnValue, it may be nice be convert it to ColumnValue::Scalar if we got Scalar::List.

Weijun-H

LGTM. thanks @guojidan

Weijun-H · 2024-03-09T03:42:54Z

datafusion/functions-array/src/udf.rs

+    array element,                                         // arg name
+    "appends an element to the end of an array.", // doc
+    array_append_udf                              // internal function name


Suggested change

array element, // arg name

"appends an element to the end of an array.", // doc

array_append_udf // internal function name

array element,

"appends an element to the end of an array.",

array_append_udf

I believe that we can get rid of these comments since the structure is apparent.

Weijun-H · 2024-03-09T03:43:23Z

datafusion/functions-array/src/udf.rs

+    element array,                                                // arg name
+    "Prepends an element to the beginning of an array.", // doc
+    array_prepend_udf                                    // internal function name


Suggested change

element array, // arg name

"Prepends an element to the beginning of an array.", // doc

array_prepend_udf // internal function name

element array,

"Prepends an element to the beginning of an array.",

array_prepend_udf

Weijun-H · 2024-03-09T03:43:51Z

datafusion/functions-array/src/udf.rs

+    "Concatenates arrays.", // doc
+    array_concat_udf        // internal function name


Suggested change

"Concatenates arrays.", // doc

array_concat_udf // internal function name

"Concatenates arrays.",

array_concat_udf

Weijun-H · 2024-03-09T03:44:15Z

datafusion/functions-array/src/udf.rs

+    "Returns an Arrow array using the specified input expressions.", // doc
+    make_array_udf // internal function name


Suggested change

"Returns an Arrow array using the specified input expressions.", // doc

make_array_udf // internal function name

"Returns an Arrow array using the specified input expressions.",

make_array_udf

jayzhan211 · 2024-03-09T03:46:56Z

make_scalar_function_with_hints

but maybe we should call another name, it is not the same as make_scalar_function_with_hints in physical expr.

jayzhan211 · 2024-03-09T03:54:03Z

@Weijun-H I think we can merge this and improve others on follow up PR

jayzhan211 · 2024-03-09T03:54:36Z

but I have no idea why the test is running endless

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-03-09T05:47:33Z

I want to split them to different files to avoid crazy conflict

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-03-09T07:11:08Z

Upd2: I found out the reason and solution.
Upd: select array_ndims([[[[[[[[[[[[[[[[[[[[[1]]]]]]]]]]]]]]]]]]]]]); is running super slow

It seems super slow to run array.slt with this PR

2031259 has been canceled after 6hr, so the issue might be the change in this PR.

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 · 2024-03-10T13:16:43Z

@guojidan Besides fixing conflicts. I split those functions to their own files, and rename to make_scalar_funciton (there should be a better name).

let me merge this PR first, and file another PR for any other changes.

Thanks @guojidan and @Weijun-H

move array function

2031259

github-actions bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates optimizer Optimizer rules labels Mar 8, 2024

guojidan mentioned this pull request Mar 8, 2024

move make_array array_append array_prepend array_concat function to datafusion-functions-array crate #9343

Closed

jayzhan211 approved these changes Mar 8, 2024

View reviewed changes

jayzhan211 reviewed Mar 8, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/main' into move-array2

94d7022

fix rebase

cda764e

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 mentioned this pull request Mar 9, 2024

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

Closed

43 tasks

Weijun-H reviewed Mar 9, 2024

View reviewed changes

Weijun-H approved these changes Mar 9, 2024

View reviewed changes

cleanup to trigger rerun

0ba624a

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 added 3 commits March 9, 2024 14:14

split functions to different files

8cc728e

Signed-off-by: jayzhan211 <[email protected]>

Merge remote-tracking branch 'upstream/main' into move-array2

99e38ab

fix

970dd7d

Signed-off-by: jayzhan211 <[email protected]>

This was referenced Mar 9, 2024

Cache the arguments type to avoid recomputing for ScalarUDF::return_type_from_exprs #9518

Closed

Extend argument types for udf return_type_from_exprs #9522

Merged

jayzhan211 added 3 commits March 10, 2024 20:20

Merge remote-tracking branch 'upstream/main' into move-array2

db82659

fix conflict

78597d8

Signed-off-by: jayzhan211 <[email protected]>

clippy

2bc6890

Signed-off-by: jayzhan211 <[email protected]>

jayzhan211 merged commit 88187d4 into apache:main Mar 10, 2024

		"Concatenates arrays.", // doc
		array_concat_udf // internal function name

		"Returns an Arrow array using the specified input expressions.", // doc
		make_array_udf // internal function name

move make_array array_append array_prepend array_concat function to datafusion-functions-array crate #9504

move make_array array_append array_prepend array_concat function to datafusion-functions-array crate #9504

Uh oh!

Conversation

guojidan commented Mar 8, 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?

Uh oh!

guojidan commented Mar 8, 2024

Uh oh!

jayzhan211 left a comment • 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!

jayzhan211 Mar 8, 2024 • 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!

jayzhan211 commented Mar 9, 2024

Uh oh!

jayzhan211 commented Mar 9, 2024

Uh oh!

Weijun-H left a comment

Choose a reason for hiding this comment

Uh oh!

Weijun-H Mar 9, 2024

Choose a reason for hiding this comment

Uh oh!

Weijun-H Mar 9, 2024

Choose a reason for hiding this comment

Uh oh!

Weijun-H Mar 9, 2024

Choose a reason for hiding this comment

Uh oh!

Weijun-H Mar 9, 2024

Choose a reason for hiding this comment

Uh oh!

Weijun-H Mar 9, 2024

Choose a reason for hiding this comment

Uh oh!

jayzhan211 commented Mar 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayzhan211 commented Mar 9, 2024

Uh oh!

jayzhan211 commented Mar 9, 2024

Uh oh!

jayzhan211 commented Mar 9, 2024

Uh oh!

jayzhan211 commented Mar 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayzhan211 commented Mar 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jayzhan211 left a comment •

edited

Loading

jayzhan211 Mar 8, 2024 •

edited

Loading

jayzhan211 commented Mar 9, 2024 •

edited

Loading

jayzhan211 commented Mar 9, 2024 •

edited

Loading