Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: make_array support empty arguments #6593

Merged
merged 11 commits into from
Jun 12, 2023
Merged

feat: make_array support empty arguments #6593

merged 11 commits into from
Jun 12, 2023

Conversation

parkma99
Copy link
Contributor

@parkma99 parkma99 commented Jun 8, 2023 •
edited
Loading

Which issue does this PR close?

Closes #6561

Rationale for this change

What changes are included in this PR?

Are these changes tested?

add some tests in array.slt

Are there any user-facing changes?

@github-actions github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Jun 8, 2023
@parkma99 parkma99 marked this pull request as ready for review June 9, 2023 04:45
dadepo
dadepo reviewed Jun 9, 2023
View reviewed changes
datafusion/physical-expr/src/functions.rs Outdated
@@ -404,7 +404,7 @@ pub fn create_physical_fun(
Arc::new(array_expressions::array_to_string)
}
BuiltinScalarFunction::Cardinality => Arc::new(array_expressions::cardinality),
BuiltinScalarFunction::MakeArray => Arc::new(array_expressions::array),
BuiltinScalarFunction::MakeArray => Arc::new(array_expressions::array_make),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't make_array be a better, more consistent renaming for the function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this in d91f667 (it was a little tricky because make_array is also a function in arrow so some new aliasing was needed

izveigor
izveigor reviewed Jun 9, 2023
View reviewed changes
datafusion/core/tests/sqllogictests/test_files/array.slt Outdated
----
[4, 5]

# trim_array scalar function #3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# trim_array scalar function #4

izveigor
izveigor approved these changes Jun 9, 2023
View reviewed changes
Copy link
Contributor

@izveigor izveigor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @parkma99. It looks good!

@parkma99
Copy link
Contributor Author

parkma99 commented Jun 9, 2023

Thank you, @parkma99. It looks good!

Thanks for reviewing

alamb
alamb approved these changes Jun 10, 2023
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @parkma99 -- I think @dadepo and @izveigor have some good comments that would be worth considering, but also I think this PR could be merged as is

@parkma99
Copy link
Contributor Author

Thank you @parkma99 -- I think @dadepo and @izveigor have some good comments that would be worth considering, but also I think this PR could be merged as is

Thank you @alamb for reviewing.

@alamb
Copy link
Contributor

alamb commented Jun 11, 2023

I took the liberty of merging this branch from master and resolving the conflicts (which I largely created via #6612) in 0947cfd

Since I was changing stuff anyways, I also made the change suggested by @dadepo in #6593 (comment) as part of

@parkma99
Copy link
Contributor Author

I took the liberty of merging this branch from master and resolving the conflicts (which I largely created via #6612) in 0947cfd

Since I was changing stuff anyways, I also made the change suggested by @dadepo in #6593 (comment) as part of

Thank you @alamb

@alamb alamb merged commit d3ef6c8 into apache:main Jun 12, 2023
jayzhan211 pushed a commit to jayzhan211/datafusion that referenced this pull request Jun 12, 2023
@parkma99 @alamb @jayzhan211
feat: make_array support empty arguments (apache#6593)
3b8d46b 
* feat: make_array support empty arguments

* fix fmt error

* fix error

* array_append support empty array

* array_prepend support empty make_array

* array_concat support empty make_array

* fix clippy

* update

* fix

* rename `array_make` --> `make_array`

---------

Co-authored-by: Andrew Lamb <[email protected]>
@parkma99 parkma99 deleted the issue6561 branch June 12, 2023 14:23
@izveigor izveigor mentioned this pull request Jul 30, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

@dadepo dadepo dadepo left review comments

@izveigor izveigor izveigor approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support empty array
4 participants
@parkma99 @alamb @dadepo @izveigor