Skip to content

AbstractParsedPercentiles should use Percentile class #24160

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

Merged
tlrx merged 3 commits into from
Apr 18, 2017
Merged

AbstractParsedPercentiles should use Percentile class #24160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
72ccbe6
AbstractParsedPercentiles should use Percentile class
tlrx Apr 18, 2017
4d09825
Add assume in InternalAggregationTestCase
tlrx Apr 18, 2017
7c7c1cd
Update after Luca review
tlrx Apr 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .../org/elasticsearch/search/aggregations/metrics/percentiles/AbstractParsedPercentiles.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public boolean hasNext() {
@Override
public Percentile next() {
Map.Entry<Double, Double> next = iterator.next();
return new InternalPercentile(next.getKey(), next.getValue());
return new Percentile(next.getKey(), next.getValue());
}
};
}
Expand Down
19 changes: 19 additions & 0 deletions ...g/elasticsearch/search/aggregations/metrics/percentiles/hdr/ParsedHDRPercentileRanks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.search.aggregations.metrics.percentiles.AbstractParsedPercentiles;
import org.elasticsearch.search.aggregations.metrics.percentiles.ParsedPercentileRanks;
import org.elasticsearch.search.aggregations.metrics.percentiles.Percentile;

import java.io.IOException;
import java.util.Iterator;

public class ParsedHDRPercentileRanks extends ParsedPercentileRanks {

Expand All @@ -33,6 +35,23 @@ protected String getType() {
return InternalHDRPercentileRanks.NAME;
}

@Override
public Iterator<Percentile> iterator() {
final Iterator<Percentile> iterator = super.iterator();
return new Iterator<Percentile>() {
@Override
public boolean hasNext() {
return iterator.hasNext();
}

@Override
public Percentile next() {
Percentile percentile = iterator.next();
return new Percentile(percentile.getValue(), percentile.getPercent());
}
};
}

private static ObjectParser<ParsedHDRPercentileRanks, Void> PARSER =
new ObjectParser<>(ParsedHDRPercentileRanks.class.getSimpleName(), true, ParsedHDRPercentileRanks::new);
static {
Expand Down
19 changes: 19 additions & 0 deletions ...csearch/search/aggregations/metrics/percentiles/tdigest/ParsedTDigestPercentileRanks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.search.aggregations.metrics.percentiles.AbstractParsedPercentiles;
import org.elasticsearch.search.aggregations.metrics.percentiles.ParsedPercentileRanks;
import org.elasticsearch.search.aggregations.metrics.percentiles.Percentile;

import java.io.IOException;
import java.util.Iterator;

public class ParsedTDigestPercentileRanks extends ParsedPercentileRanks {

Expand All @@ -33,6 +35,23 @@ protected String getType() {
return InternalTDigestPercentileRanks.NAME;
}

@Override
public Iterator<Percentile> iterator() {
final Iterator<Percentile> iterator = super.iterator();
return new Iterator<Percentile>() {
@Override
public boolean hasNext() {
return iterator.hasNext();
}

@Override
public Percentile next() {
Percentile percentile = iterator.next();
return new Percentile(percentile.getValue(), percentile.getPercent());
}
};
}

private static ObjectParser<ParsedTDigestPercentileRanks, Void> PARSER =
new ObjectParser<>(ParsedTDigestPercentileRanks.class.getSimpleName(), true, ParsedTDigestPercentileRanks::new);
static {
Expand Down
9 changes: 4 additions & 5 deletions core/src/test/java/org/elasticsearch/search/aggregations/InternalAggregationTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
import static java.util.Collections.singletonMap;
import static org.elasticsearch.common.xcontent.XContentHelper.toXContent;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;
import static org.hamcrest.Matchers.containsString;

public abstract class InternalAggregationTestCase<T extends InternalAggregation> extends AbstractWireSerializingTestCase<T> {

Expand Down Expand Up @@ -169,6 +168,10 @@ public final void testFromXContent() throws IOException {
final NamedXContentRegistry xContentRegistry = xContentRegistry();
final T aggregation = createTestInstance();

//norelease Remove this assumption when all aggregations can be parsed back.
assumeTrue("This test does not support the aggregation type yet",
getNamedXContents().stream().filter(entry -> entry.name.match(aggregation.getType())).count() > 0);

final ToXContent.Params params = new ToXContent.MapParams(singletonMap(RestSearchAction.TYPED_KEYS_PARAM, "true"));
final boolean humanReadable = randomBoolean();
final XContentType xContentType = randomFrom(XContentType.values());
Expand Down Expand Up @@ -199,10 +202,6 @@ public final void testFromXContent() throws IOException {
final BytesReference parsedBytes = toXContent((ToXContent) parsedAggregation, xContentType, params, humanReadable);
assertToXContentEquivalent(originalBytes, parsedBytes, xContentType);
assertFromXContent(aggregation, (ParsedAggregation) parsedAggregation);

} catch (NamedXContentRegistry.UnknownNamedObjectException e) {
//norelease Remove this catch block when all aggregations can be parsed back.
assertThat(e.getMessage(), containsString("Unknown Aggregation"));
}
}

Expand Down
42 changes: 42 additions & 0 deletions ...asticsearch/search/aggregations/metrics/percentiles/InternalPercentilesRanksTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,26 @@

package org.elasticsearch.search.aggregations.metrics.percentiles;

import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.rest.action.search.RestSearchAction;
import org.elasticsearch.search.DocValueFormat;
import org.elasticsearch.search.aggregations.Aggregation;
import org.elasticsearch.search.aggregations.InternalAggregation;
import org.elasticsearch.search.aggregations.InternalAggregationTestCase;
import org.elasticsearch.search.aggregations.ParsedAggregation;
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;

import java.io.IOException;
import java.util.Iterator;
import java.util.List;
import java.util.Map;

import static java.util.Collections.singletonMap;
import static org.elasticsearch.common.xcontent.XContentHelper.toXContent;

public abstract class InternalPercentilesRanksTestCase<T extends InternalAggregation> extends InternalAggregationTestCase<T> {

@Override
Expand Down Expand Up @@ -64,5 +75,36 @@ protected final void assertFromXContent(T aggregation, ParsedAggregation parsedA
assertTrue(parsedClass.isInstance(parsedAggregation));
}

public void testPercentilesRanksIterators() throws IOException {
Copy link
Member

@javanna javanna Apr 18, 2017

Choose a reason for hiding this comment

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

maybe some code could be shared, or maybe shuffling could be disabled optionally?

Copy link
Member Author

@tlrx tlrx Apr 19, 2017

Choose a reason for hiding this comment

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

I addressed this in #24183 where I added a parse() method that accepts a shuffled boolean.

Copy link
Member

@javanna javanna Apr 19, 2017

Choose a reason for hiding this comment

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

I saw it, thanks!

final T aggregation = createTestInstance();

final ToXContent.Params params = new ToXContent.MapParams(singletonMap(RestSearchAction.TYPED_KEYS_PARAM, "true"));
final XContentType xContentType = randomFrom(XContentType.values());
final BytesReference originalBytes = toXContent(aggregation, xContentType, params, randomBoolean());

Aggregation parsedAggregation;
try (XContentParser parser = xContentType.xContent().createParser(xContentRegistry(), originalBytes)) {
assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken());
assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken());

String currentName = parser.currentName();
int i = currentName.indexOf(InternalAggregation.TYPED_KEYS_DELIMITER);
String aggType = currentName.substring(0, i);
String aggName = currentName.substring(i + 1);

parsedAggregation = parser.namedObject(Aggregation.class, aggType, aggName);

assertEquals(XContentParser.Token.END_OBJECT, parser.currentToken());
assertEquals(XContentParser.Token.END_OBJECT, parser.nextToken());
assertNull(parser.nextToken());
}

final Iterator<Percentile> it = ((PercentileRanks) aggregation).iterator();
final Iterator<Percentile> parsedIt = ((PercentileRanks) parsedAggregation).iterator();
while (it.hasNext()) {
assertEquals(it.next(), parsedIt.next());
}
}

protected abstract Class<? extends ParsedPercentileRanks> parsedParsedPercentileRanksClass();
}