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.

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

Enable concurrency for multi terms agg #102710

Merged
merged 7 commits into from Nov 28, 2023

Conversation

javanna
Copy link
Member

@javanna javanna commented Nov 28, 2023

This commit enables concurrency for multi terms agg if all sub-terms agg target low cardinality fields.

This commit enables concurrency for multi terms agg if all sub-terms agg
target low cardinality fields.
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Nov 28, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

@@ -168,64 +167,52 @@ private List<BucketOrder> randomOrder() {

public void testSupportsParallelCollection() {
{
AggregatorFactories.Builder builder = new AggregatorFactories.Builder();
Copy link
Member Author

Choose a reason for hiding this comment

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

I realized that I don't need to test AggregatorFactories.Builder here, hence I simplified the test while I was at it.

return false;
}
});
assertFalse(builder.supportsParallelCollection(null));
Copy link
Member Author

Choose a reason for hiding this comment

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

Here I added a missing test that verifies that we still look at sub-aggs for composite agg, when all the sources support concurrency.

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@javanna javanna merged commit 77eb342 into elastic:main Nov 28, 2023
14 checks passed
@javanna javanna deleted the enhancement/multi_terms_concurrency branch November 28, 2023 19:33
timgrein pushed a commit to timgrein/elasticsearch that referenced this pull request Nov 30, 2023
This commit enables concurrency for multi terms agg if all sub-terms agg
target low cardinality fields.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants