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

add merge_tdigest function #20520

Merged
merged 1 commit into from
Aug 11, 2023
Merged

Conversation

ampampamp
Copy link
Contributor

@ampampamp ampampamp commented Aug 8, 2023

Description

Adding a scalar function to merge T-digests from an input array.

Motivation and Context

Existing issue: #18402

The aggregate function MERGE has existed for digests, but never the scalar equivalent.

(previously opened in #20510 but closed due to rebase error)

Impact

No impact, net-new function.

Test Plan

Added unit tests.

Built with:

./mvnw clean install -Dtest=TestTDigestFunctions -Dmaven.javadoc.skip=true -T1C -fn -pl presto-main

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes

  • Adding the function MERGE_TDIGEST.
    • Usage: MERGE_TDIGEST(CAST(ARRAY[digest1, digest2, ...] AS ARRAY(TDIGEST(DOUBLE))))

@ampampamp ampampamp requested a review from a team as a code owner August 8, 2023 21:37
@tdcmeehan
Copy link
Contributor

CC: @steveburnett to review docs change

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (documentation change only)

@ampampamp
Copy link
Contributor Author

Updated to include suggestion from @tdcmeehan.

@ampampamp
Copy link
Contributor Author

I need to address the failing checks, but @tdcmeehan - @jainavi17 had suggested the name MERGE_TDIGEST over ARRAY_MERGE that I currently have here. Thoughts?

@tdcmeehan
Copy link
Contributor

Agreed on the naming

@ampampamp ampampamp changed the title add arraymerge tdigest function add merge_tdigest function Aug 11, 2023
@github-actions
Copy link

Codenotify: Notifying subscribers in CODENOTIFY files for diff 459a8b0...2d0ad86.

Notify File(s)
@steveburnett presto-docs/src/main/sphinx/functions/tdigest.rst

@ampampamp
Copy link
Contributor Author

Updated function name to MERGE_TDIGEST.

@tdcmeehan tdcmeehan merged commit 9985b95 into prestodb:master Aug 11, 2023
55 checks passed
@wanglinsong wanglinsong mentioned this pull request Oct 3, 2023
54 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.

None yet

3 participants