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

+
+ 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

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


default_element might be more concise?

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

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

+ 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

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

+
+ 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-actionsBot 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

Comment threadpackage.json Outdated

jayzhan211 reviewed Jan 5, 2024
View reviewed changes

Comment threaddatafusion/common/src/utils.rs Outdated

jayzhan211 reviewed Jan 5, 2024
View reviewed changes

Comment threaddatafusion/physical-expr/src/array_expressions.rs Outdated

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

Comment threaddatafusion/physical-expr/src/array_expressions.rs Outdated

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

Comment threaddatafusion/common/src/utils.rs Outdated
Comment threaddatafusion/physical-expr/src/array_expressions.rs
Comment threaddatafusion/physical-expr/src/array_expressions.rs Outdated

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 cba3269Compare January 10, 2024 06:15

jayzhan211 reviewed Jan 10, 2024
View reviewed changes

Comment threaddatafusion/physical-expr/src/array_expressions.rs Outdated

jayzhan211 reviewed Jan 10, 2024
View reviewed changes

Comment threaddatafusion/physical-expr/src/array_expressions.rs Outdated

jayzhan211 reviewed Jan 10, 2024
View reviewed changes

Comment threaddatafusion/physical-expr/src/array_expressions.rs Outdated

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:mainJan 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

Uh oh!

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!

jayzhan211Jan 4, 2024

Choose a reason for hiding this comment

Uh oh!

jayzhan211Jan 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!

compheadJan 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!

jayzhan211Jan 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