Skip to content

Refactor ApproximateMostFrequent to use type annotations#21418

Merged
tdcmeehan merged 1 commit intoprestodb:masterfrom
npochhi:npochhi_approx_most_frequent_refactor
Dec 22, 2023
Merged

Refactor ApproximateMostFrequent to use type annotations#21418
tdcmeehan merged 1 commit intoprestodb:masterfrom
npochhi:npochhi_approx_most_frequent_refactor

Conversation

@npochhi
Copy link
Contributor

@npochhi npochhi commented Nov 19, 2023 •
edited
Loading

Description

Resolves #21365. This PR refactors ApproximateMostFrequent function to use function annotation framework.

Motivation and Context

This will simplify the implementation of the function.

Impact

None

Test Plan

Current tests

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

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

== NO RELEASE NOTE ==

@npochhi npochhi requested a review from a team as a code owner November 19, 2023 07:48
@npochhi npochhi requested a review from presto-oss November 19, 2023 07:48
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 19, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: npochhi / name: Nilay Pochhi (4474b3d)

@npochhi npochhi marked this pull request as draft November 19, 2023 07:57
@tdcmeehan tdcmeehan self-assigned this Nov 20, 2023
ZacBlanco
ZacBlanco reviewed Nov 22, 2023
View reviewed changes
Copy link
Contributor

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

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

two nits, otherwise LGTM

@npochhi npochhi changed the title Refactor ApproxMostFrequent to use type annotations Refactor ApproximateMostFrequent to use type annotations Nov 25, 2023
@npochhi npochhi marked this pull request as ready for review November 25, 2023 12:32
@ZacBlanco
Copy link
Contributor

ZacBlanco commented Nov 27, 2023

Can you please squash all changes into a single commit?

@npochhi npochhi force-pushed the npochhi_approx_most_frequent_refactor branch from 770249d to 435fd45 Compare November 27, 2023 07:07
@npochhi npochhi requested a review from a team as a code owner November 27, 2023 07:07
@github-actions
Copy link

github-actions bot commented Nov 27, 2023 •
edited
Loading

Codenotify: Notifying subscribers in CODENOTIFY files for diff 8aebd34...4474b3d.

No notifications.

@npochhi npochhi force-pushed the npochhi_approx_most_frequent_refactor branch 2 times, most recently from bcf2b75 to dde851c Compare November 27, 2023 07:17
ZacBlanco
ZacBlanco approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@ZacBlanco ZacBlanco left a comment

Choose a reason for hiding this comment

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

LGTM, cc: @tdcmeehan

ZacBlanco
ZacBlanco reviewed Nov 27, 2023
View reviewed changes
...-main/src/main/java/com/facebook/presto/metadata/BuiltInTypeAndFunctionNamespaceManager.java Outdated
Copy link
Contributor

@ZacBlanco ZacBlanco Nov 27, 2023

Choose a reason for hiding this comment

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

You might need to rebase on the latest master. This shouldn't be changed in this PR

Copy link
Contributor Author

@npochhi npochhi Nov 28, 2023

Choose a reason for hiding this comment

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

Yeah, sorry. Will resolve this first thing tomorrow.

@tdcmeehan
Copy link
Contributor

tdcmeehan commented Nov 28, 2023

LGTM, % need to rebase. Please, once rebased, I can merge @npochhi.

@tdcmeehan
Copy link
Contributor

tdcmeehan commented Dec 15, 2023

@npochhi will you be rebasing?

@tdcmeehan tdcmeehan force-pushed the npochhi_approx_most_frequent_refactor branch from dde851c to 4474b3d Compare December 21, 2023 19:13
@tdcmeehan
Copy link
Contributor

tdcmeehan commented Dec 21, 2023

FYI I just rebased to fix the merge conflict, will merge once tests are green.

@tdcmeehan tdcmeehan merged commit 8ac5ef7 into prestodb:master Dec 22, 2023
@feilong-liu feilong-liu mentioned this pull request Jan 17, 2024
Merged
@feilong-liu
Copy link
Contributor

feilong-liu commented Jan 17, 2024 •
edited
Loading

The aggregation function will fail for null input after this change, I am reverting the change in #21712, test query which fails can be found in the reverting PR

@tdcmeehan tdcmeehan mentioned this pull request Jan 17, 2024
Closed
@tdcmeehan
Copy link
Contributor

tdcmeehan commented Jan 17, 2024

Thanks @feilong-liu, reopened #21365.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ZacBlanco ZacBlanco ZacBlanco approved these changes

@tdcmeehan tdcmeehan tdcmeehan approved these changes

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

Assignees

@tdcmeehan tdcmeehan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use @TypeParameter to simplify approx_most_frequent aggregation function

4 participants

@npochhi @ZacBlanco @tdcmeehan @feilong-liu