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.

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

Adding support of MAP_REMOVE_NULL_VALUES for Presto #18675

Merged
merged 1 commit into from
Dec 14, 2022

Conversation

jainavi17
Copy link
Contributor

@jainavi17 jainavi17 commented Nov 13, 2022

Adding support of MAP_REMOVE_NULL_VALUES for Presto.

map_remove_null_values(map(K, V)) -> map(K, V)

Test plan
Added unit tests.
Build successfully using the following terminal command

  • mvn -Dtest=TestMapRemoveNullValuesFunction test
== RELEASE NOTES ==

General Changes
* Add :func:`map_remove_null_values` to remove all the entries where the value is null from the given map

@jainavi17 jainavi17 requested a review from a team as a code owner November 13, 2022 21:57
@jainavi17 jainavi17 requested a review from presto-oss November 13, 2022 21:57
@v-jizhang
Copy link
Contributor

@bot kick off tests

@kaikalur
Copy link
Contributor

Maybe call it MAP_REMOVE_NULL_VALUES? Not sure.

Also add documentation

@jainavi17
Copy link
Contributor Author

Maybe call it MAP_REMOVE_NULL_VALUES? Not sure.

Also add documentation

I think MAP_REMOVE_NULLS also works given keys can't be null. Also users can always refer to documentation if they have doubts.

Let me add documentation.

@jainavi17 jainavi17 force-pushed the map_remove_nulls branch 2 times, most recently from ab98a21 to 68c35c6 Compare November 17, 2022 23:13
@jainavi17
Copy link
Contributor Author

Documentation added.

@kaikalur
Copy link
Contributor

kaikalur commented Dec 6, 2022

Maybe call it MAP_REMOVE_NULL_VALUES? Not sure.
Also add documentation

I think MAP_REMOVE_NULLS also works given keys can't be null. Also users can always refer to documentation if they have doubts.

Let me add documentation.

No we should be explicit as far as possible users should understand without any other knowledge. So I say call it MAP_REMOVE_NULL_VALUES

@jainavi17
Copy link
Contributor Author

hould understand witho

No we should be explicit as far as possible users should understand without any other knowledge. So I say call it MAP_REMOVE_NULL_VALUES

OK

@jainavi17 jainavi17 force-pushed the map_remove_nulls branch 3 times, most recently from 30c6827 to 0842bc0 Compare December 8, 2022 08:35
@jainavi17 jainavi17 changed the title Adding support of MAP_REMOVE_NULLS for Presto Adding support of MAP_REMOVE_NULL_VALUES for Presto Dec 8, 2022
presto-docs/src/main/sphinx/functions/map.rst Outdated Show resolved Hide resolved
@kaikalur kaikalur requested a review from highker December 8, 2022 14:55
@jainavi17
Copy link
Contributor Author

Can we merge this one?

@kaikalur kaikalur merged commit 9c5970d into prestodb:master Dec 14, 2022
@jainavi17 jainavi17 deleted the map_remove_nulls branch December 15, 2022 02:53
@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.

4 participants