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

Adding support of MAP_TOP_N for Presto #18698

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Adding support of MAP_TOP_N for Presto #18698

merged 1 commit into from
Jan 6, 2023

Conversation

jainavi17
Copy link
Contributor

@jainavi17 jainavi17 commented Nov 17, 2022 •
edited
Loading

Adding support of MAP_TOP_N for Presto.

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

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

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

General Changes
* Add :func:`map_top_n` to truncates map items. Keeps only the top N elements by value.

@jainavi17 jainavi17 requested a review from a team as a code owner November 17, 2022 12:56
@jainavi17 jainavi17 requested a review from presto-oss November 17, 2022 12:56
@jainavi17 jainavi17 force-pushed the map_top_n_keys_by_value branch from 5614ea2 to 5773716 Compare November 17, 2022 13:08
@jainavi17 jainavi17 force-pushed the map_top_n_keys_by_value branch from 079fa55 to 563f69d Compare November 17, 2022 23:03
@kaikalur
Copy link
Contributor

kaikalur commented Dec 6, 2022

I say just do MAP_TOP_N which returns a (sub-)map and then people can do map_keys/map_values on that if needed.

@jainavi17 jainavi17 force-pushed the map_top_n_keys_by_value branch 2 times, most recently from 8715c8b to 97e615a Compare December 10, 2022 08:35
@jainavi17 jainavi17 changed the title Adding support of MAP_TOP_N_KEYS_BY_VALUE for Presto Adding support of MAP_TOP_N for Presto Dec 12, 2022
@jainavi17 jainavi17 force-pushed the map_top_n_keys_by_value branch from af1b8ed to 648d1d3 Compare December 13, 2022 07:41
@jainavi17
Adding support of MAP_TOP_N for Presto
a53f550 
rewriting the function to return top n map entries by value
@jainavi17 jainavi17 force-pushed the map_top_n_keys_by_value branch from 648d1d3 to a53f550 Compare December 15, 2022 02:48
@jainavi17
Copy link
Contributor Author

@rschlussel and @jainxrohit please review this PR. Its been pending for a very long time now. Thanks in advance!

@kaikalur kaikalur merged commit 3cfe62f into prestodb:master Jan 6, 2023
@wanglinsong wanglinsong mentioned this pull request Jan 12, 2023
Merged
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaneja aaneja aaneja left review comments

@rschlussel rschlussel rschlussel approved these changes

@kaikalur kaikalur kaikalur approved these changes

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

@jainxrohit jainxrohit Awaiting requested review from jainxrohit

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jainavi17 @kaikalur @aaneja @rschlussel