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

Prevent instantiation of top_metrics when sub-aggregations are present #96180

Merged
merged 8 commits into from
May 23, 2023
Merged

Prevent instantiation of top_metrics when sub-aggregations are present #96180

merged 8 commits into from
May 23, 2023

Conversation

shans96
Copy link
Contributor

@shans96 shans96 commented May 16, 2023 •
edited
Loading

Took inspiration from some of the other inheritors of AbstractAggregationBuilder to fix this bug, although I'm not sure if I need to override any of the other subAggregation methods inside the top metrics builder to completely fix the issue.

I was also wondering if there's a way to make use of the existing createTestInstance method to reduce the size of the new test, but it doesn't seem like I can use it because I need to set the aggregator name in advance.

Closes #95663

@elasticsearchmachine elasticsearchmachine added v8.9.0 needs:triage Requires assignment of a team area label external-contributor Pull request authored by a developer outside the Elasticsearch team labels May 16, 2023
@iverase iverase added :Analytics/Aggregations Aggregations and removed needs:triage Requires assignment of a team area label labels May 20, 2023
@iverase iverase self-assigned this May 20, 2023
@iverase iverase added the >bug label May 20, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 20, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

iverase
iverase requested changes May 20, 2023
View reviewed changes
Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

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

Thanks! could you please remove the overrided method #subAggregation(AggregationBuilder aggregation) as it is not needed. After that we can merge the change.

...src/main/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregationBuilder.java Outdated

@Override
public TopMetricsAggregationBuilder subAggregation(AggregationBuilder aggregation) {
throw new AggregationInitializationException(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is not needed and can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated

@iverase
Copy link
Contributor

iverase commented May 20, 2023 •
edited
Loading

@elasticmachine test this please

1 similar comment
@iverase
Copy link
Contributor

iverase commented May 20, 2023

@elasticmachine test this please

@shans96
Copy link
Contributor Author

shans96 commented May 20, 2023

Forgot to update test, fixing now

@iverase
Copy link
Contributor

iverase commented May 20, 2023

@elasticmachine test this please

@iverase
Copy link
Contributor

iverase commented May 20, 2023

@elasticmachine test this please

@shans96
Copy link
Contributor Author

shans96 commented May 20, 2023 •
edited
Loading

Currently fixing an issue with my local build so I can run ./gradlew check , but for now I've been relying on CI output, sorry for any inconvenience. For next time, I wanted to know if it's fine to tag elasticmachine myself if I miss something small?

@iverase
Copy link
Contributor

iverase commented May 23, 2023

@elasticmachine test this please

@iverase
Copy link
Contributor

iverase commented May 23, 2023

For next time, I wanted to know if it's fine to tag elasticmachine myself if I miss something small?

I think you cannot tag the bot, it is too clever.

@iverase
Copy link
Contributor

iverase commented May 23, 2023

@elasticmachine run elasticsearch-ci/part-1

@iverase
Copy link
Contributor

iverase commented May 23, 2023

@elasticmachine update branch

@iverase
Copy link
Contributor

iverase commented May 23, 2023

@elasticmachine test this please

iverase
iverase approved these changes May 23, 2023
View reviewed changes
Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

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

LGTM

@iverase iverase merged commit d1ec14f into elastic:main May 23, 2023
@iverase
Copy link
Contributor

iverase commented May 23, 2023

Thank you for the contribution @shans96!

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

@iverase iverase iverase approved these changes

Assignees

@iverase iverase

Labels
:Analytics/Aggregations Aggregations >bug external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.9.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

top_metrics aggregation should fail if it contains sub aggregations
4 participants
@shans96 @elasticsearchmachine @iverase @elasticmachine