Skip to content

Handle IndexOrDocValuesQuery in composite aggregation #35392

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

Merged
merged 8 commits into from
Nov 15, 2018

Conversation

jimczi
Copy link
Contributor

@jimczi jimczi commented Nov 8, 2018

The composite aggregation can optimize its execution when the query
is a match_all or a range over the field that is used in the first source
of the aggregation. However we only check for instances of PointRangeQuery whereas
the range query builder creates an IndexOrDocValuesQuery. This means that
today the optimization does not apply to range query even if the code could handle it.
This change fixes this issue by extracting the index query inside IndexOrDocValuesQuery.

The `composite` aggregation can optimize its execution when the query
is a `match_all` or a `range` over the field that is used in the first source
of the aggregation. However we only check for instances of `PointRangeQuery` whereas
the range query builder creates an  `IndexOrDocValuesQuery`. This means that
today the optimization does not apply to `range` query even if the code could handle it.
This change fixes this issue by extracting the index query inside `IndexOrDocValuesQuery`.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

@jimczi from what you state in the issue description this change makes sense to me. Left one question regarding testing, otherwise LGTM.

@@ -178,8 +180,19 @@ public void collect(int doc, long bucket) throws IOException {
};
}

static Query extractQuery(Query query) {
if (query instanceof BoostQuery) {
return extractQuery(((BoostQuery) query).getQuery());
Copy link
Member

Choose a reason for hiding this comment

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

Maybe its possible to cover this execution branch in the tests as well.

@jimczi jimczi merged commit c7a2c6d into elastic:master Nov 15, 2018
@jimczi jimczi deleted the composite_index_or_docvalues_query branch November 15, 2018 16:52
jimczi added a commit that referenced this pull request Nov 15, 2018
The `composite` aggregation can optimize its execution when the query
is a `match_all` or a `range` over the field that is used in the first source
of the aggregation. However we only check for instances of `PointRangeQuery` whereas
the range query builder creates an  `IndexOrDocValuesQuery`. This means that
today the optimization does not apply to `range` query even if the code could handle it.
This change fixes this issue by extracting the index query inside `IndexOrDocValuesQuery`.
jimczi added a commit that referenced this pull request Nov 15, 2018
The `composite` aggregation can optimize its execution when the query
is a `match_all` or a `range` over the field that is used in the first source
of the aggregation. However we only check for instances of `PointRangeQuery` whereas
the range query builder creates an  `IndexOrDocValuesQuery`. This means that
today the optimization does not apply to `range` query even if the code could handle it.
This change fixes this issue by extracting the index query inside `IndexOrDocValuesQuery`.
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants