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

Fix multi-level breadth-first aggregations. #10411

Merged
merged 1 commit into from Apr 9, 2015

Conversation

Projects
None yet
3 participants
@jpountz
Contributor

jpountz commented Apr 3, 2015

The refactoring in #9544 introduced a regression that broke multi-level
aggregations using breadth-first. This was due to sub-aggregators creating
deferred collectors before their parent aggregator and then the parent
aggregator trying to collect sub aggregators directly instead of going through
the deferred wrapper.

This commit fixes the issue but we should try to simplify all the pre/post
collection logic that we have.

Also breadth_first is now automatically ignored if the sub aggregators need
scores (just like we ignore execution_mode when the value does not make sense
like using ordinals on a script).

Close #9823

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Apr 8, 2015

Contributor

@colings86 Would you mind reviewing this change?

Contributor

jpountz commented Apr 8, 2015

@colings86 Would you mind reviewing this change?

@@ -138,6 +138,9 @@ public void replay(long... selectedBuckets) throws IOException {
this.selectedBuckets = hash;
collector.preCollection();
if (collector.needsScores()) {
throw new ElasticsearchIllegalStateException("Cannot defer if scores are needed");
}

This comment has been minimized.

@colings86

colings86 Apr 8, 2015

Member

Is there any way to do this check earlier? maybe in the preCollection method? Only wondering as we will have already done the main collect phase by this point so it would be nice to fail faster if we can

@colings86

colings86 Apr 8, 2015

Member

Is there any way to do this check earlier? maybe in the preCollection method? Only wondering as we will have already done the main collect phase by this point so it would be nice to fail faster if we can

This comment has been minimized.

@jpountz

jpountz Apr 8, 2015

Contributor

This is already checked earlier in TermsAggregator.shouldDefer. I see it as an invariant check.

@jpountz

jpountz Apr 8, 2015

Contributor

This is already checked earlier in TermsAggregator.shouldDefer. I see it as an invariant check.

This comment has been minimized.

@colings86

colings86 Apr 8, 2015

Member

ok, fair enough then

@colings86

colings86 Apr 8, 2015

Member

ok, fair enough then

@@ -531,37 +529,6 @@ public void testFailWithSubAgg() throws Exception {
assertThat(e.getMessage(), containsString("Aggregator [top_tags_hits] of type [top_hits] cannot accept sub-aggregations"));
}
}
@Test

This comment has been minimized.

@colings86

colings86 Apr 8, 2015

Member

As we have changed the functionality, should we now have a test which explicitly makes sure the aggregation is run when you run top hits on a breath_first terms agg? i.e. confirm that the setting is indeed ignored. Or is this already done elsewhere?

@colings86

colings86 Apr 8, 2015

Member

As we have changed the functionality, should we now have a test which explicitly makes sure the aggregation is run when you run top hits on a breath_first terms agg? i.e. confirm that the setting is indeed ignored. Or is this already done elsewhere?

@colings86

This comment has been minimized.

Show comment
Hide comment
@colings86

colings86 Apr 8, 2015

Member

@jpountz left some comments

Member

colings86 commented Apr 8, 2015

@jpountz left some comments

@jpountz

This comment has been minimized.

Show comment
Hide comment
@jpountz

jpountz Apr 8, 2015

Contributor

@colings86 Pushed a new commit

Contributor

jpountz commented Apr 8, 2015

@colings86 Pushed a new commit

@colings86

This comment has been minimized.

Show comment
Hide comment
@colings86

colings86 Apr 9, 2015

Member

LGTM

Member

colings86 commented Apr 9, 2015

LGTM

Aggregations: Fix multi-level breadth-first aggregations.
The refactoring in #9544 introduced a regression that broke multi-level
aggregations using breadth-first. This was due to sub-aggregators creating
deferred collectors before their parent aggregator and then the parent
aggregator trying to collect sub aggregators directly instead of going through
the deferred wrapper.

This commit fixes the issue but we should try to simplify all the pre/post
collection logic that we have.

Also `breadth_first` is now automatically ignored if the sub aggregators need
scores (just like we ignore `execution_mode` when the value does not make sense
like using ordinals on a script).

Close #9823

jpountz added a commit that referenced this pull request Apr 9, 2015

Merge pull request #10411 from jpountz/fix/multi_level_breadth_first
Aggregations: Fix multi-level breadth-first aggregations.

Close #10411

@jpountz jpountz merged commit 2a844fc into elastic:master Apr 9, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@jpountz jpountz deleted the jpountz:fix/multi_level_breadth_first branch Apr 9, 2015

@clintongormley clintongormley changed the title from Aggregations: Fix multi-level breadth-first aggregations. to Fix multi-level breadth-first aggregations. Jun 8, 2015

@clintongormley clintongormley added the >bug label Jun 8, 2015

@clintongormley clintongormley removed the review label Aug 7, 2015

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