A nested nested aggregation falls outside of its parent nested aggregation bounds
#5728
Conversation
martijnvg
changed the title
nested aggregation don't respect its parent nested aggregation.nested aggregation doesn't respect its parent nested aggregation.
| return (NestedAggregator) parent; | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
we need to remember to update this when we add the reverse_nested aggregator :)
There was a problem hiding this comment.
heh true :) maybe we turn into a useful utility? Something like: findClosestAggregator(Class) which find the first agg of a specific impl up the hierarchy.
|
Good catch! |
| // So at the time a nested 'nested' aggs is parsed its closest parent nested aggs hasn't been constructed | ||
| // so we lookup its child filter when we really need it and that time the closest parent nested aggs | ||
| // has been created and we can use its child filter as the parent filter. | ||
| parentFilterNotCached = new ClosestNestedParentFilter(closestNestedAggregator); |
There was a problem hiding this comment.
I think we need a better fix for this: it's bad that we pass the parent aggregator in but it's not fully constructed yet. Additionally, I'm concerned that the equals method of ClosestNestedParentFilter is not symetric, this could lead to duplicates in the filter cache.
There was a problem hiding this comment.
The ClosestNestedParentFilter#equals is invoked once the parent agg is completely constructed (FilterCacheFilterWrapper#getDocIdSet). I agree it is tricky, but I think the fix is ok? Maybe we need an assertion to double check.
There was a problem hiding this comment.
I think it is not OK because of the lack of symetry of the equals method: although new ClosestNestedParentFilter(parentAggregator).equals(parentFilter) would return true, parentFilter.equals(new ClosestNestedParentFilter(parentAggregator)) would return false, which means we might pollute the filter cache with a new filter that will never be reused every time this aggregation is used. So I think we need to fix the root issue, that is that parent aggregators should be fully constructed before constructing sub aggregators, before fixing this one.
There was a problem hiding this comment.
Ok, I see what you mean. The same issue then also exists in the nested query and filter, since it uses a similar mechanism to resolve the actual parent filter later on in the query execution.
|
Hi Martijn, Many thanks for this. I've tested this with v1.1.0 (excluding the test case which couldn't be merged). kind regards |
|
I hope this is the right way to use the aggregation. Please let me know if there's a better way. Thanks |
|
@dazraf That way of using aggregations looks all right to me. |
…ound the symmetry issue of ClosestNestedParentFilter#equals (this filter has been removed now)
|
@jpountz I tried to get around the symmetry issue you just described by setting the parentFilter field lazily in the setNextReader() method. I don't think that this is the nicest fix (parentFilter field is not final and gets set lazily), but I think it is good enough. |
|
Agreed that it is good enough now. +1 to merge. Thanks @martijnvg for fixing this! |
martijnvg
changed the title
nested aggregation doesn't respect its parent nested aggregation.nested aggregation falls outside of its parent nested aggregation bounds
…ent `nested` aggregation bounds. The nested `nested` aggs now gets the proper parent docs that define its bounds correctly. Closes #5728
…ent `nested` aggregation bounds. The nested `nested` aggs now gets the proper parent docs that define its bounds correctly. Closes #5728
…ent `nested` aggregation bounds. The nested `nested` aggs now gets the proper parent docs that define its bounds correctly. Closes #5728
…ent `nested` aggregation bounds. The nested `nested` aggs now gets the proper parent docs that define its bounds correctly. Closes elastic#5728
…ent `nested` aggregation bounds. The nested `nested` aggs now gets the proper parent docs that define its bounds correctly. Closes elastic#5728
clintongormley
added
:Search/Search
Every
nestedaggs uses the non nested docs filter as parent reference. This can lead to incorrect doc counts and other metrics.This PR sets the proper parent docs for nested
nestedaggs, which depends on which is the closest parent nested aggs.