Skip to content

Add SQL function array_frequency#16198

Merged
rongrong merged 1 commit intoprestodb:masterfrom
jzhou1318:master
Jun 16, 2021
Merged

Add SQL function array_frequency#16198
rongrong merged 1 commit intoprestodb:masterfrom
jzhou1318:master

Conversation

@jzhou1318
Copy link
Contributor

@jzhou1318 jzhou1318 commented Jun 3, 2021

Test plan - Wrote unit tests to check the function works with varchar and bigints. Can deal with duplicates.

Please make sure your submission complies with our Development, Formatting, and Commit Message guidelines. Don't forget to follow our attribution guidelines for any code copied from other projects.

Fill in the release notes towards the bottom of the PR description.
See Release Notes Guidelines for details.

== RELEASE NOTES ==

General Changes
* Add function :func:`array_frequency'

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 3, 2021

CLA Signed

The committers are authorized under a signed CLA.

  • ✅ Jennifer Zhou (438697a2cf5980bc4972518c29718f1c2991ad8c, f0ef293691c117c5122bcf336874a7cca3bf0de0, 4929161b18d93a5873551ada5e7702f17bf25ee6, 7221180fa78d62c665c9b2e21af396075cc713c0)

@jzhou1318 jzhou1318 requested review from kaikalur and rongrong June 3, 2021 17:27
@jzhou1318 jzhou1318 force-pushed the master branch 2 times, most recently from 7940f8d to cbe412a Compare June 11, 2021 17:01
Copy link
Contributor

@prithvip prithvip left a comment

Choose a reason for hiding this comment

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

LGTM, other than adding tests and a style nit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also add tests for the empty array case, and for nulls in the array.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also add test for empty array as @prithvip suggested. array_frequency(cast(array[] as array(bigint). Also, for bigint / varchar, either use all upper case or all lower case. SQL is case in sensitive, I think here you can use all lower case.

@jzhou1318 jzhou1318 force-pushed the master branch 3 times, most recently from 39f4b2f to 146ad59 Compare June 14, 2021 18:15
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, while this behavior is sort of reasonable, I wonder whether this is expected. @kaikalur what do you think should be the right outcome if the input array only has null elements?

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems reasonable (especially because we can't represent null as a key in the result map)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the code to ignore null elements (can deal with arrays that have both null and non-null elements), so array of nulls now returns an empty map.

@rongrong
Copy link
Contributor

We also need to update the array.rst file to add documentation for this function.

@rongrong
Copy link
Contributor

For release note, it should end with backquote rather than double quote.

Copy link
Contributor

Choose a reason for hiding this comment

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

again, lowercase bigint

Copy link
Contributor

Choose a reason for hiding this comment

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

lower case varchar

Copy link
Contributor

Choose a reason for hiding this comment

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

lowercase map(bigint, int)

Copy link
Contributor

Choose a reason for hiding this comment

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

lowercase map(varchar, int)

Generally we use () rather than <> in type signature.

Copy link
Contributor

@rongrong rongrong left a comment

Choose a reason for hiding this comment

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

Looks good! One final nit: Please change the commit title to Add SQL function array_frequency (We normally use the verb "add" when adding new features, also upper case first letter)

@jzhou1318 jzhou1318 changed the title Add array_frequency SQL function Add SQL function array_frequency Jun 15, 2021
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

@rongrong rongrong merged commit 5ae86ff into prestodb:master Jun 16, 2021
@mayankgarg1990 mayankgarg1990 mentioned this pull request Jun 18, 2021
2 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