feat: column support for `array_append`, `array_prepend`, `array_position` and `array_positions` by izveigor · Pull Request #6805 · apache/datafusion

izveigor · 2023-06-30T09:22:32Z

Which issue does this PR close?

Part of #6804

Rationale for this change

This is the first PR for solving the column issue.

What changes are included in this PR?

Four array functions now start to support column data:
array_append,
array_prepend,
array_position,
array_positions

Are these changes tested?

Yes

Are there any user-facing changes?

Yes

…ay_position and array_positions

izveigor · 2023-07-03T13:05:57Z

+ }
+ None => {
+ return Err(DataFusionError::Internal(
+ "initial position must not be null".to_string(),


PostgreSQL does not support NULL like the start position. Should we do that or is it better to translate NULL to0?

I think following postgres seems reasonable to me

izveigor · 2023-07-03T13:09:10Z

+ Some(arr) => {
+ let child_array = downcast_arg!(arr, $ARRAY_TYPE);
+ values = downcast_arg!(
+ compute::concat(&[


arrow-rs does not support into_builder method for GenericListArray. Is the concat method better than builder. Or is there a great third option? 🤔

When concatenating two arrays I don't think there is any difference.
however, you can see in the concat implementation it uses MutableArrayData: https://docs.rs/arrow-select/43.0.0/src/arrow_select/concat.rs.html#86-91 -- maybe that is an approach to pursue?
@tustvold do you know of any reason we can't add a info_builder method for GenericListArray? If not, I'll file a ticket request and see if anyone in the community wants to try.

There is no major reason why we couldn't support into_builder for ListArray, however, I should highlight this only works if there are no other references. I've not read the rest of the PR but this may not be something we can rely on here. MutableArrayData seems like the best option here if there is no selection kernel (concat, take, interleave) that suits

izveigor · 2023-07-03T13:20:39Z

 ----
 [] [] [[], []]

+# make_array with columns #1


All outputs match PostgreSQL engine results.
See the examples: #6804

We can automatically ensure they stay in sync by using the postgres compatability mode if you wanted: https://github.com/apache/arrow-datafusion/tree/main/datafusion/core/tests/sqllogictests#running-tests-postgres-compatibility

izveigor · 2023-07-03T13:23:33Z

Ready for the review @alamb
Problem solution will be divided into several independent PR so as not to cause the same problem that you mentioned in #6384

alamb · 2023-07-03T19:38:09Z

This is on my list to review, but I might not have time for it until later in the week (Wednesday) -- I am somewhat occupied with #6800 at the moment

alamb

Thank you @izveigor -- I went through this PR and it all looks great. I had a few minor comments but nothing I think should stop it from merging

alamb · 2023-07-05T16:57:19Z

 ----
 [] [] [[], []]

+# make_array with columns #1


We can automatically ensure they stay in sync by using the postgres compatability mode if you wanted: https://github.com/apache/arrow-datafusion/tree/main/datafusion/core/tests/sqllogictests#running-tests-postgres-compatibility

alamb · 2023-07-05T16:59:02Z

+ BuiltinScalarFunction::ArrayPosition => Ok(UInt64),
 BuiltinScalarFunction::ArrayPositions => {
- Ok(List(Arc::new(Field::new("item", UInt8, true))))
+ Ok(List(Arc::new(Field::new("item", UInt64, true))))


alamb · 2023-07-05T17:02:36Z

+ Some(arr) => {
+ let child_array = downcast_arg!(arr, $ARRAY_TYPE);
+ values = downcast_arg!(
+ compute::concat(&[


When concatenating two arrays I don't think there is any difference.
however, you can see in the concat implementation it uses MutableArrayData: https://docs.rs/arrow-select/43.0.0/src/arrow_select/concat.rs.html#86-91 -- maybe that is an approach to pursue?
@tustvold do you know of any reason we can't add a info_builder method for GenericListArray? If not, I'll file a ticket request and see if anyone in the community wants to try.

alamb · 2023-07-05T17:04:03Z

+ }
+ None => {
+ return Err(DataFusionError::Internal(
+ "initial position must not be null".to_string(),


I think following postgres seems reasonable to me

alamb · 2023-07-05T17:04:29Z

+ }
+ }
+ None => {
+ return Err(DataFusionError::Internal(


I think this error can be generated by bad user input (and not a bug in DataFusion) so I don't think it should be an internal error:
Suggested change
returnErr(DataFusionError::Internal(
returnErr(DataFusionError::Execution(

alamb · 2023-07-05T19:34:20Z

Thanks @izveigor

…tion` and `array_positions` (apache#6805) * test: sqllogictests with columns for array_append, array_prepend, array_position and array_positions * feat: column support for array_append and array_prepend * feat: column support for array_position and array_positions * fix: error type

izveigor marked this pull request as draft June 30, 2023 09:22

github-actionsBot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Jun 30, 2023

izveigor changed the title ~~feat: column support for array functions~~feat: column support for array_append, array_prepend, array_position and array_positionsJul 3, 2023

izveigor added 3 commits July 3, 2023 14:20

test: sqllogictests with columns for array_append, array_prepend, arr…
2941e71
…ay_position and array_positions

feat: column support for array_append and array_prepend
2becd85

feat: column support for array_position and array_positions
340bcdc

github-actionsBot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates labels Jul 3, 2023

izveigor commented Jul 3, 2023
View reviewed changes

izveigor marked this pull request as ready for review July 3, 2023 13:20

alamb approved these changes Jul 5, 2023
View reviewed changes

izveigor added 2 commits July 5, 2023 21:58

Merge branch 'main' into array_functions_with_columns
92f4240

fix: error type
fc9b93a

alamb merged commit 9a27d84 into apache:mainJul 5, 2023

izveigor mentioned this pull request Jul 6, 2023
Column support for array functions #6804
Closed
11 tasks

izveigor mentioned this pull request Jul 16, 2023
feat: array functions treat an array as an element #6986
Merged

	returnErr(DataFusionError::Internal(
	returnErr(DataFusionError::Execution(

Uh oh!

Conversation

izveigor commented Jun 30, 2023• 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

izveigor commented Jul 3, 2023

Uh oh!

alamb commented Jul 3, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Jul 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

izveigor commented Jun 30, 2023•
edited
Loading