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

Add parsing methods for Percentiles aggregations #24183

Merged

Conversation

Projects
None yet
3 participants
@tlrx
Copy link
Member

commented Apr 19, 2017

Note: pull request against feature branch

This pull request adds the parsing methods for InternalHDRPercentiles and InternalTDigestPercentiles aggregations. Parsing code is straightforward because it uses the ParsedPercentiles class created in #23974 for percentiles ranks aggregations.

Most of the changes are in test classes where code has been mutualized between classes.

@javanna
Copy link
Member

left a comment

left a couple of minors, LGTM otherwise

Aggregation parsedAggregation;
try (XContentParser parser = xContentType.xContent().createParser(xContentRegistry, originalBytes)) {
try (XContentParser parser = xContentType.xContent().createParser(xContentRegistry(), originalBytes)) {

This comment has been minimized.

Copy link
@javanna

javanna Apr 19, 2017

Member

createParser from the base test class?

assertIterators(aggregation, parse(aggregation, randomFrom(XContentType.values()), randomBoolean(), false));
}

private void assertIterators(Iterable<Percentile> aggregation, Iterable<Percentile> parsedAggregation) {

This comment has been minimized.

Copy link
@javanna

javanna Apr 19, 2017

Member

static?

This comment has been minimized.

Copy link
@cbuescher

cbuescher Apr 19, 2017

Member

I'd also merge it with the testPercentilesIterators() method if this is the only place where it is used.

This comment has been minimized.

Copy link
@tlrx

tlrx Apr 20, 2017

Author Member

Ok

@cbuescher
Copy link
Member

left a comment

@tlrx great change and nice to see how far you could unify all those aggregation and tests. I left a few minor nits and a question, nothing big that should stop you from merging this into the feature branch


private double[] percents;
private boolean keyed;
private DocValueFormat docValueFormat;

This comment has been minimized.

Copy link
@cbuescher

cbuescher Apr 19, 2017

Member

Is there a reason these three fields need to be test instance members be randomized in the init() method? Otherwise I would prfer making them local in createTestIntstance().

This comment has been minimized.

Copy link
@tlrx

tlrx Apr 20, 2017

Author Member

When reducing percentiles aggregations it is important that all reduced aggregations have the same set of percentiles otherwise the reduce phase will just fail. Before adding the parsing method, the test used a fixed 0.5 percent but I think it's better to have one or more randomized percentiles.

This comment has been minimized.

Copy link
@tlrx

tlrx Apr 20, 2017

Author Member

And I find it more rationale that all aggregations have the same format and keyed too, because it shouldn't be different from one or the other during the aggregation reducing phase.

This comment has been minimized.

Copy link
@cbuescher

cbuescher Apr 20, 2017

Member

Thanks, that makes sense. I didn't think about the reducing tests.

for (int i = 0; i < numValues; ++i) {
values[i] = randomDouble();
}
return createTestInstance(name, pipelineAggregators, metaData, keyed, docValueFormat, percents, values);

This comment has been minimized.

Copy link
@cbuescher

cbuescher Apr 19, 2017

Member

I'd prefer percents/keyed/docValueFormat to be declared and instantiated here if there is no other reason I am missing atm.

assertIterators(aggregation, parse(aggregation, randomFrom(XContentType.values()), randomBoolean(), false));
}

private void assertIterators(Iterable<Percentile> aggregation, Iterable<Percentile> parsedAggregation) {

This comment has been minimized.

Copy link
@cbuescher

cbuescher Apr 19, 2017

Member

I'd also merge it with the testPercentilesIterators() method if this is the only place where it is used.

return new InternalHDRPercentileRanks(name, cdfValues, state, keyed, format, aggregators, metadata);
boolean keyed, DocValueFormat format, double[] percents, double[] values) {
final DoubleHistogram state = new DoubleHistogram(3);
Arrays.stream(values).forEach(state::recordValue);

This comment has been minimized.

Copy link
@cbuescher

cbuescher Apr 19, 2017

Member

I was trying to understand why this step got necessary, can you briefly explain? I'm just curious what happens here since it didn't happen in the test before.

This comment has been minimized.

Copy link
@tlrx

tlrx Apr 20, 2017

Author Member

That was a mistake, now it is fixed.

@tlrx tlrx merged commit 11da773 into elastic:feature/client_aggs_parsing Apr 20, 2017

@tlrx

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2017

Thanks a lot @javanna @cbuescher !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.