Skip to content

Add REMOVE_NULLS function for arrays#18460

Merged
kaikalur merged 1 commit intoprestodb:masterfrom
sviscaino:feature/array-remove-nulls
Oct 24, 2022
Merged

Add REMOVE_NULLS function for arrays#18460
kaikalur merged 1 commit intoprestodb:masterfrom
sviscaino:feature/array-remove-nulls

Conversation

@sviscaino
Copy link
Contributor

@sviscaino sviscaino commented Oct 6, 2022

Currently, to remove nulls from an array one needs to write FILTER(arr, x -> x IS NOT NULL).
This creates a new REMOVE_NULLS function to strip nulls from an array.
Test plan -

mvn -Dtest="TestArrayOperators" test

== RELEASE NOTES ==

General Changes
* Add :func:`remove_nulls` function to remove null elements from an array

@sviscaino sviscaino requested a review from a team as a code owner October 6, 2022 15:54
@sviscaino sviscaino requested a review from presto-oss October 6, 2022 15:54
@sviscaino sviscaino force-pushed the feature/array-remove-nulls branch from 01e7bed to 44b2ff2 Compare October 6, 2022 16:08
@kaikalur
Copy link
Contributor

kaikalur commented Oct 6, 2022

This might cause a lot of issues as we are using equality (=) operator for most array operations. So NULL should not be allowed here like this.

I suggest adding a new operator REMOVE_NULLS that does it.

@sviscaino sviscaino force-pushed the feature/array-remove-nulls branch 2 times, most recently from 9053d7c to 8fbce37 Compare October 19, 2022 19:35
@sviscaino sviscaino changed the title Allow array_remove to remove nulls Add REMOVE_NULLS function for arrays Oct 19, 2022
@sviscaino
Copy link
Contributor Author

This might cause a lot of issues as we are using equality (=) operator for most array operations. So NULL should not be allowed here like this.

I suggest adding a new operator REMOVE_NULLS that does it.

That makes sense! I have updated the PR code and description accordingly.

@sviscaino sviscaino force-pushed the feature/array-remove-nulls branch from 614fd74 to 05d0984 Compare October 19, 2022 21:08
@sviscaino sviscaino force-pushed the feature/array-remove-nulls branch from 05d0984 to 6aedf19 Compare October 20, 2022 14:55
@sviscaino sviscaino force-pushed the feature/array-remove-nulls branch from 6aedf19 to 12a1c14 Compare October 24, 2022 10:47
Copy link
Contributor

@kaikalur kaikalur left a comment

Choose a reason for hiding this comment

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

Also, please add documentation for the new function:

https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/functions/array.rst

Just like for the others.

@sviscaino sviscaino requested review from kaikalur and removed request for presto-oss October 24, 2022 15:20
@sviscaino sviscaino force-pushed the feature/array-remove-nulls branch 2 times, most recently from 076722b to 842af39 Compare October 24, 2022 15:26
@sviscaino
Copy link
Contributor Author

Also, please add documentation for the new function:

https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/functions/array.rst

Just like for the others.

Yes I have added documentation in array.rst

Copy link
Contributor

@kaikalur kaikalur left a comment

Choose a reason for hiding this comment

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

LGTM

@kaikalur
Copy link
Contributor

Please fix the build - checkstyle violations.

@sviscaino sviscaino force-pushed the feature/array-remove-nulls branch from 842af39 to 99f6c57 Compare October 24, 2022 18:30
@sviscaino
Copy link
Contributor Author

Please fix the build - checkstyle violations.

Oops, done

@kaikalur kaikalur requested a review from highker October 24, 2022 19:15
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

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

LGTM; @kaikalur feel free to merge this PR

@kaikalur kaikalur merged commit 4e96f4d into prestodb:master Oct 24, 2022
@wanglinsong wanglinsong mentioned this pull request Jan 12, 2023
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants