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

Aggregation cleanup #5699

Closed
wants to merge 1 commit into from
Closed

Aggregation cleanup #5699

wants to merge 1 commit into from

Conversation

uboness
Copy link
Contributor

@uboness uboness commented Apr 6, 2014

  • consolidated value source parsing under a single parser that is reused in all the value source aggs parsers
  • consolidated include/exclude parsing under a single parser
  • cleaned up value format handling, to have consistent behaviour across all values source aggs

extendedBounds.processAndValidate(name, aggregationContext.searchContext(), parser);
roundedBounds = extendedBounds.round(rounding);
}
return new HistogramAggregator(name, factories, rounding, order, keyed, minDocCount, roundedBounds, valuesSource, formatter, parser, 50, histogramFactory, aggregationContext, parent);
ValueFormatter formatter = config.format() != null ? config.format().formatter() : null;
return new HistogramAggregator(name, factories, rounding, order, keyed, minDocCount, roundedBounds, valuesSource, formatter, 50, histogramFactory, aggregationContext, parent);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have helper methods on the config object to get the parser and formatter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

@jpountz
Copy link
Contributor

jpountz commented Apr 7, 2014

Very nice, I really like how you share the parsing of the values sources!

- consolidated value source parsing under a single parser that is reused in all the value source aggs parsers
- consolidated include/exclude parsing under a single parser
- cleaned up value format handling, to have consistent behaviour across all values source aggs
@jpountz
Copy link
Contributor

jpountz commented Apr 7, 2014

LGTM

@uboness uboness closed this Apr 7, 2014
@uboness uboness deleted the enhance/aggs_cleanup branch April 7, 2014 12:55
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

3 participants