-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Rewrite most aggregations as annotated functions #11477
Conversation
cc @radek-starburst @lukasz-stec @skrzypo987 |
core/trino-main/src/main/java/io/trino/operator/aggregation/minmaxbyn/MaxByNStateFactory.java
Outdated
Show resolved
Hide resolved
561c20f
to
e14d83a
Compare
e14d83a
to
1a6dbd6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commits through "Convert qdigest_agg and merge aggregations to annotated functions" look good
core/trino-main/src/main/java/io/trino/operator/aggregation/ChecksumAggregationFunction.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commits through "Convert avg(REAL) aggregation to annotated function" look good
core/trino-spi/src/test/java/io/trino/spi/function/TestScalarFunctionAdapter.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/test/java/io/trino/spi/function/TestScalarFunctionAdapter.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/test/java/io/trino/spi/function/TestScalarFunctionAdapter.java
Outdated
Show resolved
Hide resolved
...trino-main/src/main/java/io/trino/operator/aggregation/AggregationFromAnnotationsParser.java
Outdated
Show resolved
Hide resolved
...trino-main/src/main/java/io/trino/operator/aggregation/AggregationFromAnnotationsParser.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/aggregation/state/InOutStateSerializer.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/aggregation/state/StateCompiler.java
Outdated
Show resolved
Hide resolved
...trino-main/src/main/java/io/trino/operator/aggregation/AggregationFromAnnotationsParser.java
Outdated
Show resolved
Hide resolved
...trino-main/src/main/java/io/trino/operator/aggregation/AggregationFromAnnotationsParser.java
Outdated
Show resolved
Hide resolved
...trino-main/src/main/java/io/trino/operator/aggregation/AggregationFromAnnotationsParser.java
Outdated
Show resolved
Hide resolved
...trino-main/src/main/java/io/trino/operator/aggregation/AggregationFromAnnotationsParser.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/annotations/TypeImplementationDependency.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/aggregation/minmaxn/MaxNStateFactory.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/aggregation/minmaxn/MinNStateFactory.java
Outdated
Show resolved
Hide resolved
...no-main/src/main/java/io/trino/operator/aggregation/minmaxbyn/MinByNAggregationFunction.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/aggregation/minmaxbyn/MinByNStateFactory.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/metadata/SqlAggregationFunction.java
Outdated
Show resolved
Hide resolved
Similar to filtered aggregates, ordered aggregates can involved new symbols not in the original function call, and this optimizer does not support that model.
The typed states were added for old min/max_by aggregations, and these constructors are incompatible with the new generic state system.
a363a9c
to
e3af9f6
Compare
e3af9f6
to
7c24aa0
Compare
Optional<Class<?>> nativeContainerType = Arrays.stream(annotations) | ||
.filter(SqlType.class::isInstance) | ||
.map(SqlType.class::cast) | ||
.findFirst() | ||
.map(SqlType::nativeContainerType); | ||
// Note: this cannot be done as a chain due to strange generic type mismatches | ||
if (nativeContainerType.isPresent() && !nativeContainerType.get().equals(Object.class)) { | ||
parameterType = nativeContainerType.get(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Challenge accepted ;)
Optional<Class<?>> nativeContainerType = Arrays.stream(annotations) | |
.filter(SqlType.class::isInstance) | |
.map(SqlType.class::cast) | |
.findFirst() | |
.map(SqlType::nativeContainerType); | |
// Note: this cannot be done as a chain due to strange generic type mismatches | |
if (nativeContainerType.isPresent() && !nativeContainerType.get().equals(Object.class)) { | |
parameterType = nativeContainerType.get(); | |
} | |
parameterType = Arrays.stream(annotations) | |
.filter(SqlType.class::isInstance) | |
.map(SqlType.class::cast) | |
.findFirst() | |
.<Class<?>>map(SqlType::nativeContainerType) | |
.filter(not(Object.class::equals)) | |
.orElse(parameterType); |
@dain this does not split aggregation stats into smaller states, right? Hence there shouldn't be perf difference? |
Here is the report with comparison before and after this change, looks like result are stable without any regressions (maybe even slightly better). |
Description
NOTE: This is based on #11476, so skip the first few commits that overlap
Convert all of our aggregations to annotated functions (except for reduce which uses lambdas). To do this I extended the annotation system with:
Related issues, pull requests, and links
Documentation
(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
I'm not sure how much of this is visible to the SPI yet.
( ) No release notes entries required.
( ) Release notes entries required with the following suggested text: