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.

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

Update CompositeAggregationBuilder.java #36278

Closed
wants to merge 2 commits into from
Closed

Update CompositeAggregationBuilder.java #36278

wants to merge 2 commits into from

Conversation

FabioMNAraujo
Copy link

We want to fix this issue for a university project and we think these modifications are leading us in the right direction, but we need some help and more time to analyze more code.
This relates to: #28611

throw new IllegalArgumentException("[composite] aggregation cannot be used with a parent aggregation");
}
}*/
Copy link
Contributor

Choose a reason for hiding this comment

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

We cannot accept any parent here so as explained in #28611 we should check that all parent are instances of NestedAggregatorFactory. Something like:

private boolean checkParentIsNullOrNested(AggregatorFactory<?> parent) {
        if (parent == null) {
            return true;
        } else if (parent instanceof ReverseNestedAggregatorFactory) {
            return checkParentIsNullOrNested(parent.getParent());
        } else {
            return false;
        }
    }

Then we need a test that checks that nested aggregation and composite aggregation works fine together. org.elasticsearch.search.aggregations.bucket.NestedIT would be a good place to write such tests.

@colings86 colings86 added the :Analytics/Aggregations Aggregations label Dec 6, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

@FabioMNAraujo
Copy link
Author

Pinging @elastic/es-analytics-geo

i didn't know how to call the "doBuild" function. I created in the test a CompositeBuilder and a NestedBuilder and sub-aggregated the Composite to a Nested but I don't think the test I've created actually tests the changes added to the code. If anyone could help improve the testing, I'd be grateful

@javanna
Copy link
Member

javanna commented Dec 31, 2018

@jimczi would you be able to help here? Thanks!

@jimczi
Copy link
Contributor

jimczi commented Jan 7, 2019

@FabioMNAraujo I opened #36357 to handle nested fields in the composite aggregation. I could not use your pr directly because I didn't want to mess your master branch (you opened this pr against your master branch) so I hope you don't mind that I created a new pr for this.

@polyfractal
Copy link
Contributor

I think this was closed by #37178, please feel free to re-open if that's not true or I misunderstood :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants