Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow composite to be a sub-aggregation of filter #71639

Merged
merged 10 commits into from
Apr 19, 2021
Merged

Allow composite to be a sub-aggregation of filter #71639

merged 10 commits into from
Apr 19, 2021

Conversation

not-napoleon
Copy link
Member

This PR is intended to replace #38863. After this change, composite can be an arbitrarily deep child aggregation of any combination of nested and filter aggregations.

Relates to #37178

@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 13, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@not-napoleon not-napoleon added >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) and removed Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Apr 13, 2021
nik9000
nik9000 reviewed Apr 13, 2021
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Could you add something to CompositeAggregatorTests? Its nice to have this there too so its easier to use a debugger and stuff.

@nik9000
Copy link
Member

nik9000 commented Apr 13, 2021

The rest compat failure just need master merged in.

I did talk to @not-napoleon about adding a test for when the parent is just a filter agg. This is probably a silly thing to do - you can just put the query on top level query instead of the filter agg. But the code he's written allows it. And, surprise! It spits out the wrong results.

@not-napoleon
Copy link
Member Author

yeah, good call on testing that. I'm working on a fix for it now.

@not-napoleon
Remove nested field from most composite tests
9d9c15b 
Having the nested field forced the execution path onto the "slow" path,
even for non-nested tests.  This hurt our coverage.  Now we have an
explicitly nested index for testing nested stuff, and a non-nested index
for testing everything else.
@not-napoleon
WIP - I don't understand why this passes.
4e0e7b8
@not-napoleon
actually test composite, not terms
51bc02c
@not-napoleon
use the slow path when we have parent aggs
c30b8e2
@not-napoleon
Copy link
Member Author

@elasticmachine update branch

nik9000
nik9000 approved these changes Apr 19, 2021
View reviewed changes
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

@not-napoleon
Copy link
Member Author

@elasticmachine update branch

@not-napoleon not-napoleon merged commit 7b29cb3 into elastic:master Apr 19, 2021
@not-napoleon not-napoleon mentioned this pull request Apr 19, 2021
Merged
not-napoleon added a commit that referenced this pull request Apr 19, 2021
@not-napoleon
Allow composite to be a sub-aggregation of filter (#71639) #71867
79e1cb1
@dogberto
Copy link

@not-napoleon I don't see this one in the 7.13.0 release notes. Did it make it?

@not-napoleon
Copy link
Member Author

It should be there. I see it in the git log for 7.13, and we would have updated the tags on this issue if we'd bumped it forward. Not sure why it didn't make the release notes though.

@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nik9000 nik9000 nik9000 approved these changes

Assignees
No one assigned
Labels
:Analytics/Aggregations Aggregations >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v7.13.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@not-napoleon @elasticmachine @nik9000 @dogberto @jakelandis