Allows for a composite aggregation to be a child of a filter aggregation#38863
Allows for a composite aggregation to be a child of a filter aggregation#38863juliovalcarcel wants to merge 1 commit intoelastic:masterfrom juliovalcarcel:composite-nested-filter
Conversation
A composite aggregation should be allowed to be a grandchild of a nested aggregation only if it's parent is a filter aggregation. Relates to #37178
|
CC: @jimczi |
|
Pinging @elastic/es-analytics-geo |
|
Hey @polyfractal / @jimczi the cla is signed now, but it doesn't appear to be updated. Is there something else I have to do? |
|
Heya @juliovalcarcel, just chatted with our infra. Looks like you signed right when the clachecker was down for a change, so the signature slipped into the aether. Would you mind signing again? Sorry for the hassle! |
|
@polyfractal resigned |
|
Awesome, thanks @juliocamarero. Change looks fine to me, but I only have some experience with composite agg so we should wait for @jimczi too. Could you add a test to verify this works as expected? |
|
wrong julio? @juliovalcarcel :D |
|
Argh, sorry about that. 🤦♂️ |
|
@polyfractal I am not all that familiar with the test framework and didn't see an existing test for the composite as a child of a nested. If you or @jimczi could help provide me some guidance on how to set up the test within the elastic test framework I would be more than happy too! |
|
heya @juliovalcarcel sorry for keeping you on hold. I think it would be great if you could add a unit test to |
|
Are there any news on this ticket? I was wondering if it's going to be merged soon, as filtering before using composite aggregations seems to be a very nice thing to have. Thanks! |
The logic in the pr looks good but we need a test. Apologize for missing the ping but a rest test similar to 230_composite should be added to validate the logic. |
|
Is this ever going to be merged? |
|
I was working on merging this, which requires writing tests for it, and I think I found an issue with the PR as it stands. I think the current implementation allows multiple levels of nested aggs before the composite, but the proposed change would only allow a single level. Should be fixable, but I don't expect to get to it soon. |
A composite aggregation should be allowed to be a grandchild of a nested aggregation only if it's parent is a filter aggregation.
A nested aggregation has no way to filter a list of nested documents like you do when the composite is the top level aggregation.
Relates to #37178