Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
public final class InternalBinaryRange
extends InternalMultiBucketAggregation<InternalBinaryRange, InternalBinaryRange.Bucket>
implements Range {

public static class Bucket extends InternalMultiBucketAggregation.InternalBucket implements Range.Bucket {

private final transient DocValueFormat format;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.search.aggregations.bucket.range;

import org.elasticsearch.common.xcontent.ObjectParser;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentParserUtils;
import org.elasticsearch.search.aggregations.Aggregation;
import org.elasticsearch.search.aggregations.Aggregations;
import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation;
import org.elasticsearch.search.aggregations.bucket.range.ip.IpRangeAggregationBuilder;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;

public class ParsedBinaryRange extends ParsedMultiBucketAggregation<ParsedBinaryRange.ParsedBucket> implements Range {

@Override
public String getType() {
return IpRangeAggregationBuilder.NAME;
}

@Override
public List<? extends Range.Bucket> getBuckets() {
return buckets;
}

private static ObjectParser<ParsedBinaryRange, Void> PARSER =
new ObjectParser<>(ParsedBinaryRange.class.getSimpleName(), true, ParsedBinaryRange::new);
static {
declareMultiBucketAggregationFields(PARSER,
parser -> ParsedBucket.fromXContent(parser, false),
parser -> ParsedBucket.fromXContent(parser, true));
}

public static ParsedBinaryRange fromXContent(XContentParser parser, String name) throws IOException {
ParsedBinaryRange aggregation = PARSER.parse(parser, null);
aggregation.setName(name);
return aggregation;
}

public static class ParsedBucket extends ParsedMultiBucketAggregation.ParsedBucket implements Range.Bucket {

private String key;
private String from;
private String to;

@Override
public Object getKey() {
return key;
}

@Override
public String getKeyAsString() {
return key;
}

@Override
public Object getFrom() {
return from;
}

@Override
public String getFromAsString() {
return from;
}

@Override
public Object getTo() {
return to;
}

@Override
public String getToAsString() {
return to;
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
if (isKeyed()) {
builder.startObject(key != null ? key : rangeKey(from, to));
} else {
builder.startObject();
if (key != null) {
builder.field(CommonFields.KEY.getPreferredName(), key);
}
}
if (from != null) {
builder.field(CommonFields.FROM.getPreferredName(), getFrom());
}
if (to != null) {
builder.field(CommonFields.TO.getPreferredName(), getTo());
}
builder.field(CommonFields.DOC_COUNT.getPreferredName(), getDocCount());
getAggregations().toXContentInternal(builder, params);
builder.endObject();
return builder;
}

static ParsedBucket fromXContent(final XContentParser parser, final boolean keyed) throws IOException {
final ParsedBucket bucket = new ParsedBucket();
bucket.setKeyed(keyed);
XContentParser.Token token = parser.currentToken();
String currentFieldName = parser.currentName();

String rangeKey = null;
if (keyed) {
ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser::getTokenLocation);
rangeKey = currentFieldName;
ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
}

List<Aggregation> aggregations = new ArrayList<>();
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
} else if (token.isValue()) {
if (CommonFields.KEY.getPreferredName().equals(currentFieldName)) {
bucket.key = parser.text();
} else if (CommonFields.DOC_COUNT.getPreferredName().equals(currentFieldName)) {
bucket.setDocCount(parser.longValue());
} else if (CommonFields.FROM.getPreferredName().equals(currentFieldName)) {
bucket.from = parser.text();
} else if (CommonFields.TO.getPreferredName().equals(currentFieldName)) {
bucket.to = parser.text();
}
} else if (token == XContentParser.Token.START_OBJECT) {
aggregations.add(XContentParserUtils.parseTypedKeysObject(parser, Aggregation.TYPED_KEYS_DELIMITER, Aggregation.class));
}
}
bucket.setAggregations(new Aggregations(aggregations));

if (keyed) {
if (rangeKey(bucket.from, bucket.to).equals(rangeKey)) {
bucket.key = null;
Copy link
Member

Choose a reason for hiding this comment

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

it is ok to return null here when calling getKey and getKeyAsString? Although with keyed response we actually do print out a key? Seems like this behaviour doesn't align to REST but does align with what transport client does today?

Copy link
Member Author

Choose a reason for hiding this comment

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

I might be missing something but my latest changes should align the behavior... When the key is null, a keyed response will generate a string to name the object in the XContent response but the getKey/getKeyAsString will still return null.

This is what we do here: when we stop to parse the response, we check if the response was keyed and in this case we "try to detect" if the key has been generated on purpose using the from/to values. If so, we know that the original key is null et we set it explicitly.

The InternalMultiBucketAggregationTestCase.assertBucket() takes care of verifying that the getKey/getKeyAsString methods returns the same values after the parsing

Copy link
Member

Choose a reason for hiding this comment

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

I had missed that last line :) sorry!

Copy link
Member

Choose a reason for hiding this comment

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

the thing that worries me a bit here is that we've been testing fromXContent and toXContent everywhere, but we never test the returned values through getters, assuming that they will be the same printed out as part of toXContent, which is not the case here.

Copy link
Member Author

Choose a reason for hiding this comment

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

we never test the returned values through getters, assuming that they will be the same printed out as part of toXContent, which is not the case here.

We should always test this. The InternalAggregationTestCase sub classes can override the assertFromXContent() method in order to test getters and other methods provided by the aggregation. Multi bucket ones can override assertBucket().

Copy link
Member

Choose a reason for hiding this comment

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

alright, sounds good!

} else {
bucket.key = rangeKey;
}
}
return bucket;
}

private static String rangeKey(String from, String to) {
return (from == null ? "*" : from) + '-' + (to == null ? "*" : to);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.elasticsearch.search.aggregations.bucket.missing.InternalMissingTests;
import org.elasticsearch.search.aggregations.bucket.nested.InternalNestedTests;
import org.elasticsearch.search.aggregations.bucket.nested.InternalReverseNestedTests;
import org.elasticsearch.search.aggregations.bucket.range.InternalBinaryRangeTests;
import org.elasticsearch.search.aggregations.bucket.range.InternalRangeTests;
import org.elasticsearch.search.aggregations.bucket.range.date.InternalDateRangeTests;
import org.elasticsearch.search.aggregations.bucket.range.geodistance.InternalGeoDistanceTests;
Expand Down Expand Up @@ -132,6 +133,7 @@ private static List<InternalAggregationTestCase> getAggsTests() {
aggsTests.add(new SignificantLongTermsTests());
aggsTests.add(new SignificantStringTermsTests());
aggsTests.add(new InternalScriptedMetricTests());
aggsTests.add(new InternalBinaryRangeTests());
return Collections.unmodifiableList(aggsTests);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,12 @@ private void assertMultiBucketsAggregations(Aggregation expected, Aggregation ac
}
} else {
for (MultiBucketsAggregation.Bucket expectedBucket : expectedBuckets) {
final Object expectedKey = expectedBucket.getKey();
boolean found = false;

for (MultiBucketsAggregation.Bucket actualBucket : actualBuckets) {
if (actualBucket.getKey().equals(expectedBucket.getKey())) {
final Object actualKey = actualBucket.getKey();
if ((actualKey != null && actualKey.equals(expectedKey)) || (actualKey == null && expectedKey == null)) {
found = true;
assertBucket(expectedBucket, actualBucket, false);
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,54 +23,73 @@
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.search.DocValueFormat;
import org.elasticsearch.search.aggregations.InternalAggregations;
import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation;
import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation;
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
import org.elasticsearch.test.InternalAggregationTestCase;
import org.junit.Before;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;

public class InternalBinaryRangeTests extends InternalAggregationTestCase<InternalBinaryRange> {
private Tuple<BytesRef, BytesRef>[] RANGES;
public class InternalBinaryRangeTests extends InternalRangeTestCase<InternalBinaryRange> {

private List<Tuple<BytesRef, BytesRef>> ranges;

@Override
public void setUp() throws Exception {
super.setUp();

final int numRanges = randomIntBetween(1, 10);
ranges = new ArrayList<>(numRanges);

@Before
public void randomSortedRanges() {
int numRanges = randomIntBetween(1, 10);
Tuple<BytesRef, BytesRef>[] ranges = new Tuple[numRanges];
for (int i = 0; i < numRanges; i++) {
BytesRef[] values = new BytesRef[2];
values[0] = new BytesRef(randomAlphaOfLength(15));
values[1] = new BytesRef(randomAlphaOfLength(15));
Arrays.sort(values);
ranges[i] = new Tuple(values[0], values[1]);
ranges.add(Tuple.tuple(values[0], values[1]));
}
Arrays.sort(ranges, (t1, t2) -> t1.v1().compareTo(t2.v1()));
RANGES = ranges;
}

if (randomBoolean()) {
ranges.add(Tuple.tuple(null, new BytesRef(randomAlphaOfLength(15))));
}
if (randomBoolean()) {
ranges.add(Tuple.tuple(new BytesRef(randomAlphaOfLength(15)), null));
}
if (randomBoolean()) {
ranges.add(Tuple.tuple(null, null));
}
}

@Override
protected InternalBinaryRange createTestInstance(String name, List<PipelineAggregator> pipelineAggregators,
Map<String, Object> metaData) {
boolean keyed = randomBoolean();
protected InternalBinaryRange createTestInstance(String name,
List<PipelineAggregator> pipelineAggregators,
Map<String, Object> metaData,
InternalAggregations aggregations,
boolean keyed) {
DocValueFormat format = DocValueFormat.RAW;
List<InternalBinaryRange.Bucket> buckets = new ArrayList<>();
for (int i = 0; i < RANGES.length; ++i) {

int nullKey = randomBoolean() ? randomIntBetween(0, ranges.size() -1) : -1;
for (int i = 0; i < ranges.size(); ++i) {
final int docCount = randomIntBetween(1, 100);
buckets.add(new InternalBinaryRange.Bucket(format, keyed, randomAlphaOfLength(10),
RANGES[i].v1(), RANGES[i].v2(), docCount, InternalAggregations.EMPTY));
final String key = (i == nullKey) ? null: randomAlphaOfLength(10);
buckets.add(new InternalBinaryRange.Bucket(format, keyed, key, ranges.get(i).v1(), ranges.get(i).v2(), docCount, aggregations));
}
return new InternalBinaryRange(name, format, keyed, buckets, pipelineAggregators, Collections.emptyMap());
return new InternalBinaryRange(name, format, keyed, buckets, pipelineAggregators, metaData);
}

@Override
protected Writeable.Reader<InternalBinaryRange> instanceReader() {
return InternalBinaryRange::new;
}

@Override
protected Class<? extends ParsedMultiBucketAggregation> implementationClass() {
return ParsedBinaryRange.class;
}

@Override
protected void assertReduced(InternalBinaryRange reduced, List<InternalBinaryRange> inputs) {
int pos = 0;
Expand All @@ -86,4 +105,14 @@ protected void assertReduced(InternalBinaryRange reduced, List<InternalBinaryRan
pos ++;
}
}

@Override
protected Class<? extends InternalMultiBucketAggregation.InternalBucket> internalRangeBucketClass() {
return InternalBinaryRange.Bucket.class;
}

@Override
protected Class<? extends ParsedMultiBucketAggregation.ParsedBucket> parsedRangeBucketClass() {
return ParsedBinaryRange.ParsedBucket.class;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@

import org.elasticsearch.search.aggregations.InternalAggregation;
import org.elasticsearch.search.aggregations.InternalAggregations;
import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation;
import org.elasticsearch.search.aggregations.InternalMultiBucketAggregationTestCase;
import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation;
import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation;
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
import org.junit.Before;
Expand Down Expand Up @@ -73,11 +75,16 @@ protected void assertReduced(T reduced, List<T> inputs) {
}

@Override
protected void assertBucket(MultiBucketsAggregation.Bucket expected, MultiBucketsAggregation.Bucket actual, boolean checkOrder) {
protected final void assertBucket(MultiBucketsAggregation.Bucket expected, MultiBucketsAggregation.Bucket actual, boolean checkOrder) {
super.assertBucket(expected, actual, checkOrder);

assertTrue(expected instanceof InternalRange.Bucket);
assertTrue(actual instanceof ParsedRange.ParsedBucket);
Class<?> internalBucketClass = internalRangeBucketClass();
assertNotNull("Internal bucket class must not be null", internalBucketClass);
assertTrue(internalBucketClass.isInstance(expected));

Class<?> parsedBucketClass = parsedRangeBucketClass();
assertNotNull("Parsed bucket class must not be null", parsedBucketClass);
assertTrue(parsedBucketClass.isInstance(actual));

Range.Bucket expectedRange = (Range.Bucket) expected;
Range.Bucket actualRange = (Range.Bucket) actual;
Expand All @@ -87,4 +94,8 @@ protected void assertBucket(MultiBucketsAggregation.Bucket expected, MultiBucket
assertEquals(expectedRange.getTo(), actualRange.getTo());
assertEquals(expectedRange.getToAsString(), actualRange.getToAsString());
}

protected abstract Class<? extends InternalMultiBucketAggregation.InternalBucket> internalRangeBucketClass();

protected abstract Class<? extends ParsedMultiBucketAggregation.ParsedBucket> parsedRangeBucketClass();
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.search.DocValueFormat;
import org.elasticsearch.search.aggregations.InternalAggregations;
import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation;
import org.elasticsearch.search.aggregations.ParsedMultiBucketAggregation;
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
import org.junit.Before;
Expand Down Expand Up @@ -97,4 +98,14 @@ protected Writeable.Reader<InternalRange> instanceReader() {
protected Class<? extends ParsedMultiBucketAggregation> implementationClass() {
return ParsedRange.class;
}

@Override
protected Class<? extends InternalMultiBucketAggregation.InternalBucket> internalRangeBucketClass() {
return InternalRange.Bucket.class;
}

@Override
protected Class<? extends ParsedMultiBucketAggregation.ParsedBucket> parsedRangeBucketClass() {
return ParsedRange.ParsedBucket.class;
}
}
Loading