Allow Type in agg function metadata constructors#21316
Merged
tdcmeehan merged 1 commit intoprestodb:masterfrom Nov 8, 2023
Merged
Allow Type in agg function metadata constructors#21316tdcmeehan merged 1 commit intoprestodb:masterfrom
tdcmeehan merged 1 commit intoprestodb:masterfrom
Conversation
Contributor
Author
|
cc: @tdcmeehan |
Contributor
Author
This was referenced Nov 6, 2023
tdcmeehan
reviewed
Nov 6, 2023
presto-main/src/main/java/com/facebook/presto/operator/aggregation/state/StateCompiler.java
Outdated
Show resolved
Hide resolved
ZacBlanco
force-pushed
the
upstream-parametric-agg-types
branch
2 times, most recently
from
758697f to
61fa73e
Compare
ZacBlanco
force-pushed
the
upstream-parametric-agg-types
branch
2 times, most recently
from
70b0ce5 to
be32b1c
Compare
vivek-bharathan
approved these changes
Nov 8, 2023
Contributor
There was a problem hiding this comment.
nit: let's keep indentation consistent
Contributor
Author
There was a problem hiding this comment.
This was inserted by the IntelliJ code formatter using Presto's official codestyle XML config, so I believe it was not conforming to the code style previously? Checkstyle may be missing a rule.
Contributor
Author
There was a problem hiding this comment.
Actually, it might be a bug in the IntelliJ formatter: https://youtrack.jetbrains.com/issue/WI-50684/Continuation-indent-will-sometimes-double-indent.
This changes makes some improvements to the annotation parsing for aggregation functions. One of the main downsides to the previous implementation is that parametric aggregations with defined type parameters couldn't be easily specialized as their serialization and deserialization functions usually relied upon knowing the type parameters. Thus, many of the parametric aggregation functions presto codebase don't use the @AggregationFunction and associated annotations which makes the code more complex and hard to maintain. We introduce the ability to allow the AccumulatorStateMetadata classes, AccumulatorStateSerializer and AccumulatorStateFactory, to add `Type` parameters to their constructor with proper @typeparameter annotations. This should allow any new parametric aggregation functions to be implemented using annotations rather than manually extending BuiltInSqlFunction
ZacBlanco
force-pushed
the
upstream-parametric-agg-types
branch
from
be32b1c to
6f22d50
Compare
This was referenced Nov 10, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This change introduces the ability to allow the AccumulatorStateMetadata classes, AccumulatorStateSerializer and AccumulatorStateFactory, to add
Typeparameters to their constructor with proper @typeparameter annotations. This should allow most new parametric aggregation functions to be implemented using annotations rather than manually extending SqlAggregationFunctionMotivation and Context
One of the main downsides to the previous implementation is that parametric aggregations with defined type parameters couldn't be easily specialized as their serialization and deserialization functions usually relied upon knowing the type parameters. Thus, many of the parametric aggregation functions presto codebase don't use the @AggregationFunction and associated annotations which makes the code more complex and hard to maintain.
Impact
N/A
Test Plan
A few additional unit tests
Contributor checklist
Release Notes