feat: support `array_resize` by Weijun-H · Pull Request #8744 · apache/datafusion

Weijun-H · 2024-01-04T09:52:20Z

Which issue does this PR close?

Closes #7194

Rationale for this change

Support array_resize
Support column for this function
Support LargeList

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

jayzhan211

This looks good to me, but I have a different idea about array_size.
We can apply the combination of existing array_length, array_slice, array_repeat and array_concat to resize the array. But I'm not sure is it faster or slower, is it easier to maintain or not. How do you think about this approach @Weijun-H ?

array_concat(array_slice(array, 0, min(array_length, count)), array_repeat(default_array, max(count-array_length), 0))

jayzhan211 · 2024-01-04T11:42:29Z

datafusion/physical-expr/src/array_expressions.rs

+
+    for (index, arr) in array.iter().enumerate() {
+        let count = count_array.value(index).to_usize().ok_or_else(|| {
+            DataFusionError::Execution(


nit: exec_err

jayzhan211 · 2024-01-04T11:49:03Z

datafusion/physical-expr/src/array_expressions.rs

+    array: &GenericListArray<O>,
+    count_array: &Int64Array,
+    field: &FieldRef,
+    new_element: Option<ArrayRef>,


default_element might be more concise?

package.json

Weijun-H · 2024-01-05T04:45:58Z

array_concat(array_slice(array, 0, min(array_length, count)), array_repeat(default_array, max(count-array_length), 0))

I think this approach is not a better idea, as supporting the column for max(count-array_length) in an argument is hard. However, the MutableArray could be another direction to consider, just like you did for other functions. I will try it as a follow-up PR. @jayzhan211

datafusion/common/src/utils.rs

datafusion/physical-expr/src/array_expressions.rs

comphead · 2024-01-07T21:22:15Z

thanks @Weijun-H I'll review it tomorrow more closely if no one else beats me.

comphead · 2024-01-08T16:58:33Z

datafusion/physical-expr/src/array_expressions.rs

+                let new_rows = vec![&default_value; count - remain_count];
+                rows.extend(new_rows);
+
+                let last_offset = offsets.last().copied().unwrap();


Please handle the potential error on .unwrap

comphead

Thanks @Weijun-H tests are awesome.
Im just wondering could we just concat 2 arrays(old+new one with default values) to do a resize?

I have slight concern that performance on current implementation, maybe I'm wrong

alamb

Thank you @Weijun-H and @comphead -- this is looking good

datafusion/common/src/utils.rs

datafusion/physical-expr/src/array_expressions.rs

jayzhan211 · 2024-01-10T14:01:00Z

datafusion/physical-expr/src/array_expressions.rs

+
+    for (row_index, offset_window) in array.offsets().windows(2).enumerate() {
+        let count = count_array.value(row_index).to_usize().ok_or_else(|| {
+            exec_datafusion_err!("array_resize: failed to convert size to usize")


does type conversion more like internal_err?

wasn't expected/anticipated by the implementation and that is most likely a bug (the error message even encourages users to open a bug report)

unlike array.len() != number, which is error from user.

jayzhan211

Much better with MutableArray! LGTM

comphead

lgtm thanks for using MutableBuffer @Weijun-H it looks nicer now
Will also wait for @alamb as he had some comments as well

alamb

Looks good to me -- thanks @Weijun-H and @comphead

github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Jan 4, 2024

jayzhan211 reviewed Jan 4, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

jayzhan211 reviewed Jan 5, 2024

View reviewed changes

datafusion/common/src/utils.rs Outdated Show resolved Hide resolved

jayzhan211 reviewed Jan 5, 2024

View reviewed changes

datafusion/physical-expr/src/array_expressions.rs Outdated Show resolved Hide resolved

jayzhan211 approved these changes Jan 5, 2024

View reviewed changes

alamb mentioned this pull request Jan 5, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 1, 2024 #8704

Closed

9 tasks

comphead reviewed Jan 7, 2024

View reviewed changes

datafusion/physical-expr/src/array_expressions.rs Outdated Show resolved Hide resolved

alamb mentioned this pull request Jan 8, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 8, 2024 #8786

Closed

7 tasks

comphead reviewed Jan 8, 2024

View reviewed changes

alamb reviewed Jan 9, 2024

View reviewed changes

datafusion/common/src/utils.rs Outdated Show resolved Hide resolved

datafusion/physical-expr/src/array_expressions.rs Show resolved Hide resolved

datafusion/physical-expr/src/array_expressions.rs Outdated Show resolved Hide resolved

alamb mentioned this pull request Jan 9, 2024

Remove unused array_expression.rs and SUPPORTED_ARRAY_TYPES #8807

Merged

Weijun-H added 12 commits January 10, 2024 11:06

support array_resize

bebc566

support column

5bfee70

support LargeList

0cb9160

add function discription

e75ced5

add example

9e4695d

fix ci

0bcffca

remove useless files

24c1569

rename variable and improve error

7839366

clean comment

a1e253b

rename variable

bce6b35

improve error output

f83258b

use MutableArray

cba3269

This comment was marked as outdated.

Sign in to view

Weijun-H force-pushed the feat-array-resize branch from e66acd6 to cba3269 Compare January 10, 2024 06:15

jayzhan211 reviewed Jan 10, 2024

View reviewed changes

datafusion/physical-expr/src/array_expressions.rs Outdated Show resolved Hide resolved

jayzhan211 reviewed Jan 10, 2024

View reviewed changes

datafusion/physical-expr/src/array_expressions.rs Outdated Show resolved Hide resolved

jayzhan211 reviewed Jan 10, 2024

View reviewed changes

datafusion/physical-expr/src/array_expressions.rs Outdated Show resolved Hide resolved

refactor code and reduce extra function calls

6e50702

Weijun-H requested review from alamb, comphead and jayzhan211 January 10, 2024 11:09

jayzhan211 reviewed Jan 10, 2024

View reviewed changes

jayzhan211 approved these changes Jan 10, 2024

View reviewed changes

fix error

591bd1f

comphead approved these changes Jan 10, 2024

View reviewed changes

alamb approved these changes Jan 11, 2024

View reviewed changes

alamb merged commit 8a0b447 into apache:main Jan 11, 2024

Weijun-H mentioned this pull request Jan 13, 2024

Minor: fix wrong function call #8847

Merged

alamb mentioned this pull request Jan 14, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 15, 2024 #8864

Closed

9 tasks

Conversation

Weijun-H commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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

Choose a reason for hiding this comment

Uh oh!

jayzhan211 Jan 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Weijun-H commented Jan 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

comphead commented Jan 7, 2024

Uh oh!

comphead Jan 8, 2024

Choose a reason for hiding this comment

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

jayzhan211 Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

jayzhan211 left a comment

Choose a reason for hiding this comment

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Weijun-H commented Jan 4, 2024 •

edited

Loading

jayzhan211 left a comment •

edited

Loading

Weijun-H commented Jan 5, 2024 •

edited

Loading