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
Aggregations: Fail with a parse exception if top_hits
agg is defined under a nested
agg.
#8673
Aggregations: Fail with a parse exception if top_hits
agg is defined under a nested
agg.
#8673
Conversation
public Aggregator create(AggregationContext aggregationContext, Aggregator parent, long expectedBucketsCount) { | ||
return new TopHitsAggregator(fetchPhase, topHitsContext, name, expectedBucketsCount, aggregationContext, parent); | ||
public Aggregator create(AggregationContext context, Aggregator parent, long expectedBucketsCount) { | ||
if (ReverseNestedAggregator.findClosestNestedAggregator(parent) != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct? What if there is a reverse_nested
aggregation in-between? (ie. nested > reverse_nested > top_hits)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It then still fails, because this method iterates all the way to the root agg and find that nested
agg. If no nested
agg have been defined above the reverse_nested
agg than the parsing of the reverse_nested
agg fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then it should work right? For instance if you first have the following aggregation tree:
- level 1: nested aggregation
- level 2: buckets aggregation based on some nested properties
- level 3: reverse_nested aggregation to go back to the root docs
- level 4: top_hits aggregation to get the top hits per bucket
Then the top_hits
aggregation is legal since it applies to root documents although it has a parent nested
aggregation?
@jpountz Good point. I updated the PR to take into account the fact the a |
LGTM |
…s defined under a `nested` agg. Closes elastic#8668 Closes elastic#8673
9713131
to
6029559
Compare
top_hits
agg is defined under a nested
agg.top_hits
agg is defined under a nested
agg.
…s defined under a `nested` agg. Closes elastic#8668 Closes elastic#8673
…s defined under a `nested` agg. Closes elastic#8668 Closes elastic#8673
PR for #8668