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.

Sign up for GitHub

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

Jump to bottom

ES|QL Add primitive float variants of all aggregators to the compute engine #109781

Merged
merged 24 commits into from
Jun 20, 2024
Merged

ES|QL Add primitive float variants of all aggregators to the compute engine #109781

merged 24 commits into from
Jun 20, 2024

Conversation

ChrisHegarty
Copy link
Contributor

@ChrisHegarty ChrisHegarty commented Jun 16, 2024 •
edited
Loading

This commit adds primitive float variants of all ES|QL aggregators to the compute engine.

Much of the changes are mechanical and/or are generated, and retain similar type input/output params as other aggregators.

Agg input type output type
Min/Max float float
Sum float double
Count Distinct float long
Median Abs Dev float double
Percentile float double
Top List float float
Values float float

Still no language changes or support for floats at the language level, this PR just addresses support in the engine.

relates: #109746

@ChrisHegarty ChrisHegarty added >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL v8.15.0 labels Jun 16, 2024
@ChrisHegarty ChrisHegarty requested a review from a team as a code owner June 16, 2024 14:19
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@elasticsearchmachine
Copy link
Collaborator

Hi @ChrisHegarty, I've created a changelog YAML for you.

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

alex-spies
alex-spies reviewed Jun 19, 2024
View reviewed changes
Copy link
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

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

Got a first pass; still need to review the tests in a second pass.

x-pack/plugin/esql/compute/gen/src/main/java/org/elasticsearch/compute/gen/Types.java Outdated Show resolved Hide resolved
.../esql/compute/src/main/java/org/elasticsearch/compute/aggregation/X-ValuesAggregator.java.st Show resolved Hide resolved
...ompute/src/main/java/org/elasticsearch/compute/aggregation/CountDistinctFloatAggregator.java Show resolved Hide resolved
...gin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/MinFloatAggregator.java Show resolved Hide resolved
...gin/esql/compute/src/main/java/org/elasticsearch/compute/aggregation/SumFloatAggregator.java Show resolved Hide resolved
...n/esql/compute/src/test/java/org/elasticsearch/compute/operator/ForkingOperatorTestCase.java Show resolved Hide resolved
alex-spies
alex-spies approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@alex-spies alex-spies 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 except for a couple minor comments on the tests; before merging, I think we should probably remove the copy-pasted mute, and we should check if the sum aggregation tests test what they ought to, given that the aggregation state is double not float.

...ute/src/test/java/org/elasticsearch/compute/aggregation/MaxFloatAggregatorFunctionTests.java Outdated Show resolved Hide resolved
...n/esql/compute/src/test/java/org/elasticsearch/compute/operator/ForkingOperatorTestCase.java Show resolved Hide resolved
...csearch/compute/aggregation/MedianAbsoluteDeviationFloatGroupingAggregatorFunctionTests.java Outdated Show resolved Hide resolved
...ute/src/test/java/org/elasticsearch/compute/aggregation/SumFloatAggregatorFunctionTests.java Show resolved Hide resolved
...ute/src/test/java/org/elasticsearch/compute/aggregation/SumFloatAggregatorFunctionTests.java Outdated Show resolved Hide resolved
...ute/src/test/java/org/elasticsearch/compute/aggregation/SumFloatAggregatorFunctionTests.java Show resolved Hide resolved
ChrisHegarty
ChrisHegarty commented Jun 20, 2024
View reviewed changes
Copy link
Contributor Author

@ChrisHegarty ChrisHegarty left a comment

Choose a reason for hiding this comment

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

Thanks for committing the TopList float aggregator @ivancea

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty
Copy link
Contributor Author

@elasticmachine update branch

@ChrisHegarty ChrisHegarty merged commit 2c205e3 into elastic:main Jun 20, 2024
15 checks passed
@ChrisHegarty ChrisHegarty deleted the esql_float_aggs branch June 20, 2024 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@costin costin costin left review comments

@nik9000 nik9000 nik9000 approved these changes

@alex-spies alex-spies alex-spies approved these changes

Assignees
No one assigned
Labels
:Analytics/ES|QL AKA ESQL >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.15.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@ChrisHegarty @elasticsearchmachine @costin @nik9000 @alex-spies @elasticmachine @ivancea