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

Lower the initial sizing of sub aggregations. #5994

Closed
wants to merge 2 commits into from

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Apr 30, 2014

We currently compute initial sizings based on the cardinality of our fields.
This can be highly exagerated for sub aggregations, for example if there is a
parent terms aggregation that is executed over a field that has a very long
tail: most buckets will only collect a couple of documents.

We currently compute initial sizings based on the cardinality of our fields.
This can be highly exagerated for sub aggregations, for example if there is a
parent terms aggregation that is executed over a field that has a very long
tail: most buckets will only collect a couple of documents.

Close elastic#5994
@jpountz jpountz added the review label Apr 30, 2014
@@ -25,6 +26,16 @@
*/
public abstract class AggregatorFactory {

protected static boolean hasParentBucketAggregator(Aggregator parent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't feel like it belongs to the factory... why not put it in the Aggregator class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This utility method is only useful to factories though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Putting it on the Aggregator class would require to make it public.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any problem with making it public

@jpountz
Copy link
Contributor Author

jpountz commented May 6, 2014

@uboness I just pushed a new commit to address your comments

@uboness
Copy link
Contributor

uboness commented May 6, 2014

LGTM!

@jpountz jpountz closed this in 8cd7811 May 6, 2014
jpountz added a commit that referenced this pull request May 6, 2014
We currently compute initial sizings based on the cardinality of our fields.
This can be highly exagerated for sub aggregations, for example if there is a
parent terms aggregation that is executed over a field that has a very long
tail: most buckets will only collect a couple of documents.

Close #5994
@kevinkluge kevinkluge removed the review label May 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants