From 5c09a48f991581cbbd4785ac7f684a5a9d4d3b2b Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 14 Mar 2019 14:12:51 -0400 Subject: [PATCH 01/29] Enable range field type --- .../index/fielddata/plain/DocValuesIndexFieldData.java | 3 ++- .../org/elasticsearch/index/mapper/RangeFieldMapper.java | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/plain/DocValuesIndexFieldData.java b/server/src/main/java/org/elasticsearch/index/fielddata/plain/DocValuesIndexFieldData.java index 5732a872c8f58..4c39b7ab9aecc 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/plain/DocValuesIndexFieldData.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/plain/DocValuesIndexFieldData.java @@ -30,6 +30,7 @@ import org.elasticsearch.index.mapper.IdFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.MapperService; +import org.elasticsearch.index.mapper.RangeFieldMapper; import org.elasticsearch.indices.breaker.CircuitBreakerService; import java.util.Set; @@ -87,7 +88,7 @@ public IndexFieldData build(IndexSettings indexSettings, MappedFieldType fiel CircuitBreakerService breakerService, MapperService mapperService) { // Ignore Circuit Breaker final String fieldName = fieldType.name(); - if (BINARY_INDEX_FIELD_NAMES.contains(fieldName)) { + if (BINARY_INDEX_FIELD_NAMES.contains(fieldName)|| fieldType.getClass() == RangeFieldMapper.RangeFieldType.class) { assert numericType == null; return new BinaryDVIndexFieldData(indexSettings.getIndex(), fieldName); } else if (numericType != null) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java index e5ba55de7bfd0..9a92eed1ad2a5 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java @@ -54,6 +54,8 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.support.XContentMapValues; +import org.elasticsearch.index.fielddata.IndexFieldData; +import org.elasticsearch.index.fielddata.plain.DocValuesIndexFieldData; import org.elasticsearch.index.mapper.NumberFieldMapper.NumberType; import org.elasticsearch.index.query.QueryShardContext; @@ -246,6 +248,12 @@ public int hashCode() { return Objects.hash(super.hashCode(), rangeType, dateTimeFormatter); } + @Override + public IndexFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName) { + failIfNoDocValues(); + return new DocValuesIndexFieldData.Builder(); + } + @Override public String typeName() { return rangeType.name; From 6d986f10459a6344c10baf3c81675ea361186956 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 14 Mar 2019 14:13:05 -0400 Subject: [PATCH 02/29] very minimal range histo test --- .../histogram/HistogramAggregatorTests.java | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorTests.java index bdb99a4971ac1..daa90df01d595 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.bucket.histogram; +import org.apache.lucene.document.BinaryDocValuesField; import org.apache.lucene.document.Document; import org.apache.lucene.document.SortedNumericDocValuesField; import org.apache.lucene.index.IndexReader; @@ -26,12 +27,16 @@ import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.store.Directory; +import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.NumericUtils; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.NumberFieldMapper; +import org.elasticsearch.index.mapper.RangeFieldMapper; import org.elasticsearch.search.aggregations.AggregatorTestCase; import org.elasticsearch.search.aggregations.support.AggregationInspectionHelper; +import java.util.Collections; + public class HistogramAggregatorTests extends AggregatorTestCase { public void testLongs() throws Exception { @@ -96,6 +101,32 @@ public void testDoubles() throws Exception { } } + public void testDoubleRanges() throws Exception { + + RangeFieldMapper.RangeType rangeType = RangeFieldMapper.RangeType.DOUBLE; + try (Directory dir = newDirectory(); + RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { + Document doc = new Document(); + BytesRef encodedRange = + rangeType.encodeRanges(Collections.singleton(new RangeFieldMapper.Range(rangeType, 1.0D, 3.0D, true, true))); + doc.add(new BinaryDocValuesField("field", encodedRange)); + w.addDocument(doc); + + HistogramAggregationBuilder aggBuilder = new HistogramAggregationBuilder("my_agg") + .field("field") + .interval(5); + MappedFieldType fieldType = new RangeFieldMapper.Builder("field", rangeType).fieldType(); + fieldType.setName("field"); + + try (IndexReader reader = w.getReader()) { + IndexSearcher searcher = new IndexSearcher(reader); + InternalHistogram histogram = search(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType); + assertEquals(1, histogram.getBuckets().size()); + } + + } + } + public void testIrrationalInterval() throws Exception { try (Directory dir = newDirectory(); RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { From b9f906e9d332bff397af1bfe4ba963ce3f008d30 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 26 Mar 2019 10:46:50 -0400 Subject: [PATCH 03/29] Forking the internal histogram implementations based on field type --- .../aggregations/AggregationBuilders.java | 3 +- .../HistogramAggregationBuilder.java | 14 ++-- .../histogram/HistogramAggregatorFactory.java | 29 ++++++-- ...r.java => NumericHistogramAggregator.java} | 12 ++-- .../histogram/RangeHistogramAggregator.java | 70 +++++++++++++++++++ .../aggregations/support/ValueType.java | 4 +- .../aggregations/bucket/HistogramTests.java | 5 +- ...a => NumericHistogramAggregatorTests.java} | 48 +++---------- .../RangeHistogramAggregatorTests.java | 45 ++++++++++++ .../CumulativeSumAggregatorTests.java | 7 +- .../pipeline/DerivativeAggregatorTests.java | 27 +++---- .../xpack/rollup/RollupRequestTranslator.java | 3 +- .../rollup/RollupJobIdentifierUtilTests.java | 10 +-- .../rollup/RollupRequestTranslationTests.java | 2 +- .../RollupResponseTranslationTests.java | 4 +- 15 files changed, 195 insertions(+), 88 deletions(-) rename server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/{HistogramAggregator.java => NumericHistogramAggregator.java} (92%) create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java rename server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/{HistogramAggregatorTests.java => NumericHistogramAggregatorTests.java} (89%) create mode 100644 server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java index 72ac99d94b951..8fe2736d7ec74 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java @@ -89,6 +89,7 @@ import org.elasticsearch.search.aggregations.metrics.WeightedAvgAggregationBuilder; import org.elasticsearch.search.aggregations.metrics.MedianAbsoluteDeviationAggregationBuilder; import org.elasticsearch.search.aggregations.metrics.MedianAbsoluteDeviation; +import org.elasticsearch.search.aggregations.support.ValueType; import java.util.List; import java.util.Map; @@ -245,7 +246,7 @@ public static GeoDistanceAggregationBuilder geoDistance(String name, GeoPoint or * Create a new {@link Histogram} aggregation with the given name. */ public static HistogramAggregationBuilder histogram(String name) { - return new HistogramAggregationBuilder(name); + return new HistogramAggregationBuilder(name, ValueType.DOUBLE); } /** diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java index 3be7bcd596d7b..f1a6db36bd113 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java @@ -34,7 +34,6 @@ import org.elasticsearch.search.aggregations.bucket.MultiBucketAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValueType; import org.elasticsearch.search.aggregations.support.ValuesSource; -import org.elasticsearch.search.aggregations.support.ValuesSource.Numeric; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; @@ -50,7 +49,7 @@ /** * A builder for histograms on numeric fields. */ -public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder +public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder implements MultiBucketAggregationBuilder { public static final String NAME = "histogram"; @@ -65,7 +64,7 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder< private static final ObjectParser PARSER; static { PARSER = new ObjectParser<>(HistogramAggregationBuilder.NAME); - ValuesSourceParserHelper.declareNumericFields(PARSER, true, true, false); + ValuesSourceParserHelper.declareAnyFields(PARSER, true, true); PARSER.declareDouble(HistogramAggregationBuilder::interval, Histogram.INTERVAL_FIELD); @@ -84,7 +83,7 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder< } public static HistogramAggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException { - return PARSER.parse(parser, new HistogramAggregationBuilder(aggregationName), null); + return PARSER.parse(parser, new HistogramAggregationBuilder(aggregationName, ValueType.DOUBLE), null); } private double interval; @@ -96,8 +95,8 @@ public static HistogramAggregationBuilder parse(String aggregationName, XContent private long minDocCount = 0; /** Create a new builder with the given name. */ - public HistogramAggregationBuilder(String name) { - super(name, ValuesSourceType.NUMERIC, ValueType.DOUBLE); + public HistogramAggregationBuilder(String name, ValueType valueType) { + super(name, ValuesSourceType.ANY, valueType); } protected HistogramAggregationBuilder(HistogramAggregationBuilder clone, Builder factoriesBuilder, Map metaData) { @@ -117,6 +116,7 @@ protected AggregationBuilder shallowCopy(Builder factoriesBuilder, Map innerBuild(SearchContext context, ValuesSourceConfig config, + protected ValuesSourceAggregatorFactory innerBuild(SearchContext context, ValuesSourceConfig config, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { return new HistogramAggregatorFactory(name, config, interval, offset, order, keyed, minDocCount, minBound, maxBound, context, parent, subFactoriesBuilder, metaData); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java index 683ec1987c597..3681b1f2383b9 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java @@ -25,7 +25,6 @@ import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import org.elasticsearch.search.aggregations.BucketOrder; import org.elasticsearch.search.aggregations.support.ValuesSource; -import org.elasticsearch.search.aggregations.support.ValuesSource.Numeric; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; import org.elasticsearch.search.internal.SearchContext; @@ -34,7 +33,7 @@ import java.util.List; import java.util.Map; -public final class HistogramAggregatorFactory extends ValuesSourceAggregatorFactory { +public final class HistogramAggregatorFactory extends ValuesSourceAggregatorFactory { private final double interval, offset; private final BucketOrder order; @@ -42,7 +41,7 @@ public final class HistogramAggregatorFactory extends ValuesSourceAggregatorFact private final long minDocCount; private final double minBound, maxBound; - public HistogramAggregatorFactory(String name, ValuesSourceConfig config, double interval, double offset, + public HistogramAggregatorFactory(String name, ValuesSourceConfig config, double interval, double offset, BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, SearchContext context, AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, Map metaData) throws IOException { @@ -61,24 +60,40 @@ public long minDocCount() { } @Override - protected Aggregator doCreateInternal(ValuesSource.Numeric valuesSource, Aggregator parent, boolean collectsFromSingleBucket, + protected Aggregator doCreateInternal(ValuesSource valuesSource, Aggregator parent, boolean collectsFromSingleBucket, List pipelineAggregators, Map metaData) throws IOException { if (collectsFromSingleBucket == false) { return asMultiBucketAggregator(this, context, parent); } - return createAggregator(valuesSource, parent, pipelineAggregators, metaData); + if (valuesSource instanceof ValuesSource.Numeric) { + return createAggregator((ValuesSource.Numeric) valuesSource, parent, pipelineAggregators, metaData); + } + else if (valuesSource instanceof ValuesSource.Bytes) { + return createAggregator((ValuesSource.Bytes) valuesSource, parent, pipelineAggregators, metaData); + } + else { + throw new IllegalArgumentException("Expected one of [Numeric, Bytes] values source, found [" + valuesSource.toString() + "]"); + } } private Aggregator createAggregator(ValuesSource.Numeric valuesSource, Aggregator parent, List pipelineAggregators, Map metaData) throws IOException { - return new HistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, valuesSource, + return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, valuesSource, config.format(), context, parent, pipelineAggregators, metaData); } + private Aggregator createAggregator(ValuesSource.Bytes valuesSource, Aggregator parent, List pipelineAggregators, + Map metaData) throws IOException { + + return new RangeHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, valuesSource, + config.format(), context, parent, pipelineAggregators, metaData); + } + + @Override protected Aggregator createUnmapped(Aggregator parent, List pipelineAggregators, Map metaData) throws IOException { - return createAggregator(null, parent, pipelineAggregators, metaData); + return createAggregator((ValuesSource.Numeric) null, parent, pipelineAggregators, metaData); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregator.java similarity index 92% rename from server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregator.java rename to server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregator.java index 1295cec2e4b6d..b63cf94a98085 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregator.java @@ -52,7 +52,7 @@ * written as {@code interval * x + offset} and yet is less than or equal to * {@code value}. */ -class HistogramAggregator extends BucketsAggregator { +class NumericHistogramAggregator extends BucketsAggregator { private final ValuesSource.Numeric valuesSource; private final DocValueFormat formatter; @@ -64,11 +64,11 @@ class HistogramAggregator extends BucketsAggregator { private final LongHash bucketOrds; - HistogramAggregator(String name, AggregatorFactories factories, double interval, double offset, - BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, - @Nullable ValuesSource.Numeric valuesSource, DocValueFormat formatter, - SearchContext context, Aggregator parent, - List pipelineAggregators, Map metaData) throws IOException { + NumericHistogramAggregator(String name, AggregatorFactories factories, double interval, double offset, + BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, + @Nullable ValuesSource.Numeric valuesSource, DocValueFormat formatter, + SearchContext context, Aggregator parent, + List pipelineAggregators, Map metaData) throws IOException { super(name, factories, context, parent, pipelineAggregators, metaData); if (interval <= 0) { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java new file mode 100644 index 0000000000000..d3186f88b05b9 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java @@ -0,0 +1,70 @@ +package org.elasticsearch.search.aggregations.bucket.histogram; + +import org.apache.lucene.index.LeafReaderContext; +import org.elasticsearch.common.Nullable; +import org.elasticsearch.common.util.LongHash; +import org.elasticsearch.search.DocValueFormat; +import org.elasticsearch.search.aggregations.Aggregator; +import org.elasticsearch.search.aggregations.AggregatorFactories; +import org.elasticsearch.search.aggregations.BucketOrder; +import org.elasticsearch.search.aggregations.InternalAggregation; +import org.elasticsearch.search.aggregations.InternalOrder; +import org.elasticsearch.search.aggregations.LeafBucketCollector; +import org.elasticsearch.search.aggregations.bucket.BucketsAggregator; +import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; +import org.elasticsearch.search.aggregations.support.ValuesSource; +import org.elasticsearch.search.internal.SearchContext; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +public class RangeHistogramAggregator extends BucketsAggregator { + private final ValuesSource.Bytes valuesSource; + private final DocValueFormat formatter; + private final double interval, offset; + private final BucketOrder order; + private final boolean keyed; + private final long minDocCount; + private final double minBound, maxBound; + + private final LongHash bucketOrds; + + RangeHistogramAggregator(String name, AggregatorFactories factories, double interval, double offset, + BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, + @Nullable ValuesSource.Bytes valuesSource, DocValueFormat formatter, + SearchContext context, Aggregator parent, + List pipelineAggregators, Map metaData) throws IOException { + + super(name, factories, context, parent, pipelineAggregators, metaData); + if (interval <= 0) { + throw new IllegalArgumentException("interval must be positive, got: " + interval); + } + this.interval = interval; + this.offset = offset; + this.order = InternalOrder.validate(order, this); + this.keyed = keyed; + this.minDocCount = minDocCount; + this.minBound = minBound; + this.maxBound = maxBound; + this.valuesSource = valuesSource; + this.formatter = formatter; + + bucketOrds = new LongHash(1, context.bigArrays()); + } + + @Override + protected LeafBucketCollector getLeafCollector(LeafReaderContext ctx, LeafBucketCollector sub) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public InternalAggregation buildAggregation(long bucket) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public InternalAggregation buildEmptyAggregation() { + throw new UnsupportedOperationException(); + } +} diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValueType.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValueType.java index fc23f72eddc9c..3f2c1d7510e40 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValueType.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValueType.java @@ -26,6 +26,7 @@ import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.IndexGeoPointFieldData; import org.elasticsearch.index.fielddata.IndexNumericFieldData; +import org.elasticsearch.index.fielddata.plain.BinaryDVIndexFieldData; import org.elasticsearch.index.mapper.DateFieldMapper; import org.elasticsearch.search.DocValueFormat; @@ -48,7 +49,8 @@ public enum ValueType implements Writeable { // TODO: what is the difference between "number" and "numeric"? NUMERIC((byte) 7, "numeric", "numeric", ValuesSourceType.NUMERIC, IndexNumericFieldData.class, DocValueFormat.RAW), GEOPOINT((byte) 8, "geo_point", "geo_point", ValuesSourceType.GEOPOINT, IndexGeoPointFieldData.class, DocValueFormat.GEOHASH), - BOOLEAN((byte) 9, "boolean", "boolean", ValuesSourceType.NUMERIC, IndexNumericFieldData.class, DocValueFormat.BOOLEAN); + BOOLEAN((byte) 9, "boolean", "boolean", ValuesSourceType.NUMERIC, IndexNumericFieldData.class, DocValueFormat.BOOLEAN), + RANGE((byte) 10, "range", "range",ValuesSourceType.BYTES, BinaryDVIndexFieldData.class, DocValueFormat.RAW); final String description; final ValuesSourceType valuesSourceType; diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java index ef5cb19ed6dd7..ad16bd7b2f8ee 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java @@ -22,6 +22,7 @@ import org.elasticsearch.search.aggregations.BaseAggregationTestCase; import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder; import org.elasticsearch.search.aggregations.BucketOrder; +import org.elasticsearch.search.aggregations.support.ValueType; import java.util.ArrayList; import java.util.List; @@ -33,7 +34,7 @@ public class HistogramTests extends BaseAggregationTestCase translateHistogram(HistogramAggregationB return translateVSAggBuilder(source, filterConditions, registry, () -> { HistogramAggregationBuilder rolledHisto - = new HistogramAggregationBuilder(source.getName()); + = new HistogramAggregationBuilder(source.getName(), ValueType.DOUBLE); rolledHisto.interval(source.interval()); rolledHisto.offset(source.offset()); diff --git a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtilTests.java b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtilTests.java index d05a78e121296..c8cf12cba7101 100644 --- a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtilTests.java +++ b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtilTests.java @@ -267,7 +267,7 @@ public void testComparableNoHistoVsHisto() { DateHistogramAggregationBuilder builder = new DateHistogramAggregationBuilder("foo").field("foo") .dateHistogramInterval(new DateHistogramInterval("1h")) - .subAggregation(new HistogramAggregationBuilder("histo").field("bar").interval(100)); + .subAggregation(new HistogramAggregationBuilder("histo", ValueType.DOUBLE).field("bar").interval(100)); Set caps = new HashSet<>(2); caps.add(cap); @@ -303,7 +303,7 @@ public void testComparableNoTermsVsTerms() { } public void testHistoSameNameWrongTypeInCaps() { - HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo"); + HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo", ValueType.DOUBLE); histo.field("foo") .interval(1L) .subAggregation(new MaxAggregationBuilder("the_max").field("max_field")) @@ -389,7 +389,7 @@ public void testDateHistoMissingFieldInCaps() { } public void testHistoMissingFieldInCaps() { - HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo"); + HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo", ValueType.DOUBLE); histo.interval(1) .field("foo") .subAggregation(new MaxAggregationBuilder("the_max").field("max_field")) @@ -412,7 +412,7 @@ public void testHistoMissingFieldInCaps() { } public void testNoMatchingHistoInterval() { - HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo"); + HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo", ValueType.DOUBLE); histo.interval(1) .field("bar") .subAggregation(new MaxAggregationBuilder("the_max").field("max_field")) @@ -433,7 +433,7 @@ public void testNoMatchingHistoInterval() { } public void testHistoIntervalNotMultiple() { - HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo"); + HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo", ValueType.DOUBLE); histo.interval(10) // <--- interval is not a multiple of 3 .field("bar") .subAggregation(new MaxAggregationBuilder("the_max").field("max_field")) diff --git a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupRequestTranslationTests.java b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupRequestTranslationTests.java index 1ceac98725e8b..feb45c47f9486 100644 --- a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupRequestTranslationTests.java +++ b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupRequestTranslationTests.java @@ -330,7 +330,7 @@ public void testStringTerms() throws IOException { public void testBasicHisto() { - HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo"); + HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo", ValueType.DOUBLE); histo.field("foo") .interval(1L) .extendedBounds(0.0, 1000.0) diff --git a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupResponseTranslationTests.java b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupResponseTranslationTests.java index 849461f1b6202..bf5d703f6f56b 100644 --- a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupResponseTranslationTests.java +++ b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupResponseTranslationTests.java @@ -1174,10 +1174,10 @@ public void testLongTerms() throws IOException { } public void testHisto() throws IOException { - HistogramAggregationBuilder nonRollupHisto = new HistogramAggregationBuilder("histo") + HistogramAggregationBuilder nonRollupHisto = new HistogramAggregationBuilder("histo", ValueType.DOUBLE) .field("bar").interval(100); - HistogramAggregationBuilder rollupHisto = new HistogramAggregationBuilder("histo") + HistogramAggregationBuilder rollupHisto = new HistogramAggregationBuilder("histo", ValueType.DOUBLE) .field("bar.histogram." + RollupField.VALUE) .interval(100) .subAggregation(new SumAggregationBuilder("histo." + RollupField.COUNT_FIELD) From 4ad0a0c8bc28d6df90128c42db317e2d61cbba11 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 26 Mar 2019 10:48:31 -0400 Subject: [PATCH 04/29] add apache license to new files --- .../histogram/RangeHistogramAggregator.java | 19 +++++++++++++++++++ .../RangeHistogramAggregatorTests.java | 19 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java index d3186f88b05b9..7d8cf294b7793 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java @@ -1,3 +1,22 @@ +/* + * 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.histogram; import org.apache.lucene.index.LeafReaderContext; diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java index 5c0ca5a2febe8..a6733b588bde9 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java @@ -1,3 +1,22 @@ +/* + * 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.histogram; import org.apache.lucene.document.BinaryDocValuesField; From 0b8c814089211815b37776c20739ed42dc654308 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 27 Mar 2019 15:44:18 -0400 Subject: [PATCH 05/29] outline for leaf bucket collector --- .../histogram/RangeHistogramAggregator.java | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java index 7d8cf294b7793..0e81a16978ddd 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java @@ -20,8 +20,10 @@ package org.elasticsearch.search.aggregations.bucket.histogram; import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.queries.BinaryDocValuesRangeQuery; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.util.LongHash; +import org.elasticsearch.index.fielddata.SortedBinaryDocValues; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.Aggregator; import org.elasticsearch.search.aggregations.AggregatorFactories; @@ -29,6 +31,7 @@ import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.InternalOrder; import org.elasticsearch.search.aggregations.LeafBucketCollector; +import org.elasticsearch.search.aggregations.LeafBucketCollectorBase; import org.elasticsearch.search.aggregations.bucket.BucketsAggregator; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import org.elasticsearch.search.aggregations.support.ValuesSource; @@ -74,7 +77,18 @@ public class RangeHistogramAggregator extends BucketsAggregator { @Override protected LeafBucketCollector getLeafCollector(LeafReaderContext ctx, LeafBucketCollector sub) throws IOException { - throw new UnsupportedOperationException(); + if (valuesSource == null) { + return LeafBucketCollector.NO_OP_COLLECTOR; + } + final SortedBinaryDocValues values = valuesSource.bytesValues(ctx); + // TODO: For prototyping, just hard code this to work with Double ranges + final BinaryDocValuesRangeQuery.LengthType lengthType = BinaryDocValuesRangeQuery.LengthType.FIXED_8; + return new LeafBucketCollectorBase(sub, values) { + @Override + public void collect(int doc, long bucket) throws IOException { + assert bucket == 0; + } + }; } @Override From 40a60eb812f0c68df0a42baee4430f82bfb304f3 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 24 Apr 2019 10:53:31 -0400 Subject: [PATCH 06/29] ValuesSource for Range doc values --- .../index/mapper/RangeFieldMapper.java | 2 ++ .../search/aggregations/support/ValueType.java | 2 +- .../aggregations/support/ValuesSource.java | 12 ++++++++++++ .../aggregations/support/ValuesSourceConfig.java | 16 ++++++++++++++++ .../aggregations/support/ValuesSourceType.java | 3 ++- 5 files changed, 33 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java index 9a92eed1ad2a5..3f4a8caa943cd 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java @@ -228,6 +228,8 @@ public static final class RangeFieldType extends MappedFieldType { } } + public RangeType rangeType() { return rangeType; } + @Override public MappedFieldType clone() { return new RangeFieldType(this); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValueType.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValueType.java index 3f2c1d7510e40..d130b385be89e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValueType.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValueType.java @@ -50,7 +50,7 @@ public enum ValueType implements Writeable { NUMERIC((byte) 7, "numeric", "numeric", ValuesSourceType.NUMERIC, IndexNumericFieldData.class, DocValueFormat.RAW), GEOPOINT((byte) 8, "geo_point", "geo_point", ValuesSourceType.GEOPOINT, IndexGeoPointFieldData.class, DocValueFormat.GEOHASH), BOOLEAN((byte) 9, "boolean", "boolean", ValuesSourceType.NUMERIC, IndexNumericFieldData.class, DocValueFormat.BOOLEAN), - RANGE((byte) 10, "range", "range",ValuesSourceType.BYTES, BinaryDVIndexFieldData.class, DocValueFormat.RAW); + RANGE((byte) 10, "range", "range", ValuesSourceType.RANGE, BinaryDVIndexFieldData.class, DocValueFormat.RAW); final String description; final ValuesSourceType valuesSourceType; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java index b931cd81da954..e8674b6677d10 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java @@ -42,6 +42,7 @@ import org.elasticsearch.index.fielddata.SortedNumericDoubleValues; import org.elasticsearch.index.fielddata.SortingBinaryDocValues; import org.elasticsearch.index.fielddata.SortingNumericDoubleValues; +import org.elasticsearch.index.mapper.RangeFieldMapper; import org.elasticsearch.script.AggregationScript; import org.elasticsearch.search.aggregations.support.ValuesSource.WithScript.BytesValues; import org.elasticsearch.search.aggregations.support.values.ScriptBytesValues; @@ -179,6 +180,17 @@ public FieldData(IndexFieldData indexFieldData) { public SortedBinaryDocValues bytesValues(LeafReaderContext context) { return indexFieldData.load(context).getBytesValues(); } + + public static class RangeFieldData extends FieldData { + private final RangeFieldMapper.RangeType rangeType; + + public RangeFieldData (IndexFieldData indexFieldData, RangeFieldMapper.RangeType rangeType) { + super(indexFieldData); + this.rangeType = rangeType; + } + + public RangeFieldMapper.RangeType rangeType() { return rangeType; } + } } public static class Script extends Bytes { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java index c1c171e3e4fd5..cf269829e83d6 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java @@ -28,6 +28,7 @@ import org.elasticsearch.index.fielddata.IndexOrdinalsFieldData; import org.elasticsearch.index.mapper.DateFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.index.mapper.RangeFieldMapper; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.script.AggregationScript; import org.elasticsearch.script.Script; @@ -244,6 +245,7 @@ public VS toValuesSource(QueryShardContext context) { } else if (valueSourceType() == ValuesSourceType.ANY || valueSourceType() == ValuesSourceType.BYTES) { vs = (VS) ValuesSource.Bytes.WithOrdinals.EMPTY; } else { + // TODO: Do we need a missing case for Range values type? throw new IllegalArgumentException("Can't deal with unmapped ValuesSource type " + valueSourceType()); } } else { @@ -285,6 +287,7 @@ private VS originalValuesSource() { if (valueSourceType() == ValuesSourceType.BYTES) { return (VS) bytesScript(); } + // TODO: Do we need a range script case? throw new AggregationExecutionException("value source of type [" + valueSourceType().name() + "] is not supported by scripts"); } @@ -295,6 +298,9 @@ private VS originalValuesSource() { if (valueSourceType() == ValuesSourceType.GEOPOINT) { return (VS) geoPointField(); } + if (valueSourceType() == ValuesSourceType.RANGE) { + return (VS) rangeField(); + } // falling back to bytes values return (VS) bytesField(); } @@ -344,4 +350,14 @@ private ValuesSource.GeoPoint geoPointField() { return new ValuesSource.GeoPoint.Fielddata((IndexGeoPointFieldData) fieldContext().indexFieldData()); } + + private ValuesSource rangeField() { + MappedFieldType fieldType = fieldContext.fieldType(); + + if (fieldType instanceof RangeFieldMapper.RangeFieldType == false) { + throw new IllegalStateException("Asked for range ValuesSource, but field is of type " + fieldType.name()); + } + RangeFieldMapper.RangeFieldType rangeFieldType = (RangeFieldMapper.RangeFieldType)fieldType; + return new ValuesSource.Bytes.FieldData.RangeFieldData(fieldContext().indexFieldData(), rangeFieldType.rangeType()); + } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java index a4da3e3e3c320..93398abe99e9a 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java @@ -30,7 +30,8 @@ public enum ValuesSourceType implements Writeable { ANY, NUMERIC, BYTES, - GEOPOINT; + GEOPOINT, + RANGE; public static ValuesSourceType fromString(String name) { return valueOf(name.trim().toUpperCase(Locale.ROOT)); From 164c40ea35b7468608f8089ebacf921fcb857e09 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Mon, 29 Apr 2019 10:50:33 -0400 Subject: [PATCH 07/29] =?UTF-8?q?Copy=20numeric=20histogram=20build=20aggr?= =?UTF-8?q?egation=C2=A0methods=20for=20range=20histogram?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../histogram/RangeHistogramAggregator.java | 32 +++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java index 0e81a16978ddd..d6999e7bd1401 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java @@ -21,6 +21,7 @@ import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.queries.BinaryDocValuesRangeQuery; +import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.util.LongHash; import org.elasticsearch.index.fielddata.SortedBinaryDocValues; @@ -38,6 +39,8 @@ import org.elasticsearch.search.internal.SearchContext; import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Map; @@ -91,13 +94,38 @@ public void collect(int doc, long bucket) throws IOException { }; } + // TODO: buildAggregation and buildEmptyAggregation are literally just copied out of NumericHistogramAggregator. We could refactor + // this to an abstract super class, if we wanted to. Might be overkill. @Override public InternalAggregation buildAggregation(long bucket) throws IOException { - throw new UnsupportedOperationException(); + assert bucket == 0; + consumeBucketsAndMaybeBreak((int) bucketOrds.size()); + List buckets = new ArrayList<>((int) bucketOrds.size()); + for (long i = 0; i < bucketOrds.size(); i++) { + double roundKey = Double.longBitsToDouble(bucketOrds.get(i)); + double key = roundKey * interval + offset; + buckets.add(new InternalHistogram.Bucket(key, bucketDocCount(i), keyed, formatter, bucketAggregations(i))); + } + + // the contract of the histogram aggregation is that shards must return buckets ordered by key in ascending order + CollectionUtil.introSort(buckets, BucketOrder.key(true).comparator(this)); + + InternalHistogram.EmptyBucketInfo emptyBucketInfo = null; + if (minDocCount == 0) { + emptyBucketInfo = new InternalHistogram.EmptyBucketInfo(interval, offset, minBound, maxBound, buildEmptySubAggregations()); + } + return new InternalHistogram(name, buckets, order, minDocCount, emptyBucketInfo, formatter, keyed, pipelineAggregators(), + metaData()); } @Override public InternalAggregation buildEmptyAggregation() { - throw new UnsupportedOperationException(); + InternalHistogram.EmptyBucketInfo emptyBucketInfo = null; + if (minDocCount == 0) { + emptyBucketInfo = new InternalHistogram.EmptyBucketInfo(interval, offset, minBound, maxBound, buildEmptySubAggregations()); + } + return new InternalHistogram(name, Collections.emptyList(), order, minDocCount, emptyBucketInfo, formatter, keyed, + pipelineAggregators(), metaData()); } + } From 7d93c56dd1789c3f70c3f14830614861ebe10d08 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 1 May 2019 09:52:20 -0400 Subject: [PATCH 08/29] More range values source stuff --- .../bucket/histogram/HistogramAggregatorFactory.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java index 3681b1f2383b9..41fb3341539e2 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java @@ -68,11 +68,12 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, Aggregator pare if (valuesSource instanceof ValuesSource.Numeric) { return createAggregator((ValuesSource.Numeric) valuesSource, parent, pipelineAggregators, metaData); } - else if (valuesSource instanceof ValuesSource.Bytes) { + else if (valuesSource instanceof ValuesSource.Bytes.FieldData.RangeFieldData) { return createAggregator((ValuesSource.Bytes) valuesSource, parent, pipelineAggregators, metaData); } else { - throw new IllegalArgumentException("Expected one of [Numeric, Bytes] values source, found [" + valuesSource.toString() + "]"); + throw new IllegalArgumentException("Expected one of [Numeric, RangeFieldData] values source, found [" + + valuesSource.toString() + "]"); } } From 10fee2eeddd239ce0d965f017e9d61cd1b6f3675 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 7 May 2019 11:05:55 -0400 Subject: [PATCH 09/29] Use the RangeFieldData values source in the RangeHistogram --- .../bucket/histogram/RangeHistogramAggregator.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java index d6999e7bd1401..ded66bf03bdc7 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java @@ -20,11 +20,11 @@ package org.elasticsearch.search.aggregations.bucket.histogram; import org.apache.lucene.index.LeafReaderContext; -import org.apache.lucene.queries.BinaryDocValuesRangeQuery; import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.util.LongHash; import org.elasticsearch.index.fielddata.SortedBinaryDocValues; +import org.elasticsearch.index.mapper.RangeFieldMapper; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.Aggregator; import org.elasticsearch.search.aggregations.AggregatorFactories; @@ -45,7 +45,7 @@ import java.util.Map; public class RangeHistogramAggregator extends BucketsAggregator { - private final ValuesSource.Bytes valuesSource; + private final ValuesSource.Bytes.FieldData.RangeFieldData valuesSource; private final DocValueFormat formatter; private final double interval, offset; private final BucketOrder order; @@ -57,7 +57,7 @@ public class RangeHistogramAggregator extends BucketsAggregator { RangeHistogramAggregator(String name, AggregatorFactories factories, double interval, double offset, BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, - @Nullable ValuesSource.Bytes valuesSource, DocValueFormat formatter, + @Nullable ValuesSource.Bytes.FieldData.RangeFieldData valuesSource, DocValueFormat formatter, SearchContext context, Aggregator parent, List pipelineAggregators, Map metaData) throws IOException { @@ -84,8 +84,7 @@ protected LeafBucketCollector getLeafCollector(LeafReaderContext ctx, LeafBucket return LeafBucketCollector.NO_OP_COLLECTOR; } final SortedBinaryDocValues values = valuesSource.bytesValues(ctx); - // TODO: For prototyping, just hard code this to work with Double ranges - final BinaryDocValuesRangeQuery.LengthType lengthType = BinaryDocValuesRangeQuery.LengthType.FIXED_8; + final RangeFieldMapper.RangeType rangeType = valuesSource.rangeType(); return new LeafBucketCollectorBase(sub, values) { @Override public void collect(int doc, long bucket) throws IOException { From c9d36a784fdc2e6b8ca888acee3ab7db1a0a135a Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 9 May 2019 09:27:59 -0400 Subject: [PATCH 10/29] Wire up decoder logic --- .../index/mapper/RangeFieldMapper.java | 8 ++++ .../elasticsearch/index/mapper/RangeType.java | 42 +++++++++++++++++++ .../histogram/RangeHistogramAggregator.java | 34 +++++++++++++++ 3 files changed, 84 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java index 43687ee0f6653..2c9b45b0c64ab 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java @@ -478,6 +478,14 @@ public String toString() { sb.append(includeTo ? ']' : ')'); return sb.toString(); } + + public Object getFrom() { + return from; + } + + public Object getTo() { + return to; + } } static class BinaryRangesDocValuesField extends CustomDocValuesField { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java index ac3ec8f750603..e819c60588603 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java @@ -108,6 +108,11 @@ public List decodeRanges(BytesRef bytes) { throw new UnsupportedOperationException(); } + @Override + public Double doubleValue (Object endpointValue) { + throw new UnsupportedOperationException("IP ranges cannot be safely converted to doubles"); + } + @Override public Query dvRangeQuery(String field, BinaryDocValuesRangeQuery.QueryType queryType, Object from, Object to, boolean includeFrom, boolean includeTo) { @@ -208,6 +213,11 @@ public List decodeRanges(BytesRef bytes) { return LONG.decodeRanges(bytes); } + @Override + public Double doubleValue (Object endpointValue) { + return LONG.doubleValue(endpointValue); + } + @Override public Query dvRangeQuery(String field, BinaryDocValuesRangeQuery.QueryType queryType, Object from, Object to, boolean includeFrom, boolean includeTo) { @@ -274,6 +284,12 @@ public List decodeRanges(BytesRef bytes) { return BinaryRangeUtil.decodeFloatRanges(bytes); } + @Override + public Double doubleValue(Object endpointValue) { + assert endpointValue instanceof Float; + return ((Float) endpointValue).doubleValue(); + } + @Override public Query dvRangeQuery(String field, BinaryDocValuesRangeQuery.QueryType queryType, Object from, Object to, boolean includeFrom, boolean includeTo) { @@ -339,6 +355,12 @@ public List decodeRanges(BytesRef bytes) { return BinaryRangeUtil.decodeDoubleRanges(bytes); } + @Override + public Double doubleValue(Object endpointValue) { + assert endpointValue instanceof Double; + return (Double)endpointValue; + } + @Override public Query dvRangeQuery(String field, BinaryDocValuesRangeQuery.QueryType queryType, Object from, Object to, boolean includeFrom, boolean includeTo) { @@ -407,6 +429,11 @@ public List decodeRanges(BytesRef bytes) { return LONG.decodeRanges(bytes); } + @Override + public Double doubleValue (Object endpointValue) { + return LONG.doubleValue(endpointValue); + } + @Override public Query dvRangeQuery(String field, BinaryDocValuesRangeQuery.QueryType queryType, Object from, Object to, boolean includeFrom, boolean includeTo) { @@ -461,6 +488,12 @@ public List decodeRanges(BytesRef bytes) { return BinaryRangeUtil.decodeLongRanges(bytes); } + @Override + public Double doubleValue(Object endpointValue) { + assert endpointValue instanceof Long; + return ((Long) endpointValue).doubleValue(); + } + @Override public Query dvRangeQuery(String field, BinaryDocValuesRangeQuery.QueryType queryType, Object from, Object to, boolean includeFrom, boolean includeTo) { @@ -621,6 +654,15 @@ public Query rangeQuery(String field, boolean hasDocValues, Object from, Object public abstract BytesRef encodeRanges(Set ranges) throws IOException; public abstract List decodeRanges(BytesRef bytes); + /** + * Given the Range.to or Range.from Object value from a Range instance, converts that value into a Double. Before converting, it + * asserts that the object is of the expected type. Operation is not supported on IP ranges (because of loss of precision) + * + * @param endpointValue Object value for Range.to or Range.from + * @return endpointValue as a Double + */ + public abstract Double doubleValue(Object endpointValue); + public abstract Query dvRangeQuery(String field, BinaryDocValuesRangeQuery.QueryType queryType, Object from, Object to, boolean includeFrom, boolean includeTo); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java index b1ea7254381fd..da88b28899191 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java @@ -20,10 +20,12 @@ package org.elasticsearch.search.aggregations.bucket.histogram; import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.util.LongHash; import org.elasticsearch.index.fielddata.SortedBinaryDocValues; +import org.elasticsearch.index.mapper.RangeFieldMapper; import org.elasticsearch.index.mapper.RangeType; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.Aggregator; @@ -89,6 +91,38 @@ protected LeafBucketCollector getLeafCollector(LeafReaderContext ctx, LeafBucket @Override public void collect(int doc, long bucket) throws IOException { assert bucket == 0; + if (values.advanceExact(doc)) { + // Is it possible for valuesCount to be > 1 here? Multiple ranges are encoded into the same BytesRef in the binary doc + // values, so it isn't clear what we'd be iterating over. + final int valuesCount = values.docValueCount(); + + double previousKey = Double.NEGATIVE_INFINITY; + for (int i = 0; i < valuesCount; i++) { + BytesRef encodedRanges = values.nextValue(); + // This list should be sorted by start-of-range, I think? + List ranges = rangeType.decodeRanges(encodedRanges); + for (RangeFieldMapper.Range range : ranges) { + for (double value = rangeType.doubleValue(range.getTo()); value <= rangeType.doubleValue(range.getFrom()); + value += interval) { + double key = Math.floor((value - offset) / interval); + if (key <= previousKey) { + continue; + } + // Bucket collection identical to NumericHistogramAggregator, could be refactored + long bucketOrd = bucketOrds.add(Double.doubleToLongBits(key)); + if (bucketOrd < 0) { // already seen + bucketOrd = -1 - bucketOrd; + collectExistingBucket(sub, doc, bucketOrd); + } else { + collectBucket(sub, doc, bucketOrd); + } + previousKey = key; + + } + } + + } + } } }; } From d962c1e2430977ba50d3e7048357d55bec5113e1 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 9 May 2019 11:36:57 -0400 Subject: [PATCH 11/29] Put endpoints in the right order --- .../aggregations/bucket/histogram/RangeHistogramAggregator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java index da88b28899191..623ebebd361dc 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java @@ -102,7 +102,7 @@ public void collect(int doc, long bucket) throws IOException { // This list should be sorted by start-of-range, I think? List ranges = rangeType.decodeRanges(encodedRanges); for (RangeFieldMapper.Range range : ranges) { - for (double value = rangeType.doubleValue(range.getTo()); value <= rangeType.doubleValue(range.getFrom()); + for (double value = rangeType.doubleValue(range.getFrom()); value <= rangeType.doubleValue(range.getTo()); value += interval) { double key = Math.floor((value - offset) / interval); if (key <= previousKey) { From d93139c55b5e6b1f47e63f52d62b5e87d3d968e8 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 9 May 2019 11:59:54 -0400 Subject: [PATCH 12/29] release bucketOrds hash when cleaning up --- .../bucket/histogram/RangeHistogramAggregator.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java index 623ebebd361dc..70ee982cff1f5 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java @@ -23,6 +23,7 @@ import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.common.Nullable; +import org.elasticsearch.common.lease.Releasables; import org.elasticsearch.common.util.LongHash; import org.elasticsearch.index.fielddata.SortedBinaryDocValues; import org.elasticsearch.index.mapper.RangeFieldMapper; @@ -161,4 +162,8 @@ public InternalAggregation buildEmptyAggregation() { pipelineAggregators(), metaData()); } + @Override + public void doClose() { + Releasables.close(bucketOrds); + } } From b13b0eaf25164137141e850611669ae2e9fbd33d Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Fri, 10 May 2019 10:11:19 -0400 Subject: [PATCH 13/29] Clean up DocValuesIndexFieldData kludge --- .../index/fielddata/plain/DocValuesIndexFieldData.java | 10 ++++++++-- .../elasticsearch/index/mapper/RangeFieldMapper.java | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/plain/DocValuesIndexFieldData.java b/server/src/main/java/org/elasticsearch/index/fielddata/plain/DocValuesIndexFieldData.java index 4c39b7ab9aecc..529bdb84b12ac 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/plain/DocValuesIndexFieldData.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/plain/DocValuesIndexFieldData.java @@ -30,7 +30,7 @@ import org.elasticsearch.index.mapper.IdFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.MapperService; -import org.elasticsearch.index.mapper.RangeFieldMapper; +import org.elasticsearch.index.mapper.RangeType; import org.elasticsearch.indices.breaker.CircuitBreakerService; import java.util.Set; @@ -72,6 +72,7 @@ public static class Builder implements IndexFieldData.Builder { private NumericType numericType; private Function> scriptFunction = AbstractAtomicOrdinalsFieldData.DEFAULT_SCRIPT_FUNCTION; + private RangeType rangeType; public Builder numericType(NumericType type) { this.numericType = type; @@ -83,12 +84,17 @@ public Builder scriptFunction(Function> s return this; } + public Builder setRangeType(RangeType rangeType) { + this.rangeType = rangeType; + return this; + } + @Override public IndexFieldData build(IndexSettings indexSettings, MappedFieldType fieldType, IndexFieldDataCache cache, CircuitBreakerService breakerService, MapperService mapperService) { // Ignore Circuit Breaker final String fieldName = fieldType.name(); - if (BINARY_INDEX_FIELD_NAMES.contains(fieldName)|| fieldType.getClass() == RangeFieldMapper.RangeFieldType.class) { + if (BINARY_INDEX_FIELD_NAMES.contains(fieldName) || rangeType != null) { assert numericType == null; return new BinaryDVIndexFieldData(indexSettings.getIndex(), fieldName); } else if (numericType != null) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java index 2c9b45b0c64ab..c4fe6ca934f96 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java @@ -237,7 +237,7 @@ public int hashCode() { @Override public IndexFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName) { failIfNoDocValues(); - return new DocValuesIndexFieldData.Builder(); + return new DocValuesIndexFieldData.Builder().setRangeType(rangeType); } @Override From 9af80744f4efbf4c0ecf613491a9f8ee9e6f671d Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Fri, 10 May 2019 14:04:11 -0400 Subject: [PATCH 14/29] Fix histogram serialization --- .../bucket/histogram/HistogramAggregationBuilder.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java index ef089bd9baaa2..b8408dd1afcd5 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java @@ -118,7 +118,7 @@ protected AggregationBuilder shallowCopy(Builder factoriesBuilder, Map Date: Fri, 31 May 2019 14:32:20 -0400 Subject: [PATCH 15/29] fix failing tests --- .../aggregations/metrics/ValueCountAggregatorTests.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorTests.java index ea14d9ec671b2..db900c09c98d0 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorTests.java @@ -40,6 +40,8 @@ import org.elasticsearch.index.mapper.KeywordFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.NumberFieldMapper; +import org.elasticsearch.index.mapper.RangeFieldMapper; +import org.elasticsearch.index.mapper.RangeType; import org.elasticsearch.search.aggregations.AggregatorTestCase; import org.elasticsearch.search.aggregations.support.AggregationInspectionHelper; import org.elasticsearch.search.aggregations.support.ValueType; @@ -166,6 +168,8 @@ private static MappedFieldType createMappedFieldType(ValueType valueType) { return new IpFieldMapper.Builder("_name").fieldType(); case GEOPOINT: return new GeoPointFieldMapper.Builder("_name").fieldType(); + case RANGE: + return new RangeFieldMapper.Builder("_name", RangeType.DOUBLE).fieldType(); default: throw new IllegalArgumentException("Test does not support value type [" + valueType + "]"); } From 9b7448aa16afeffb40a52d4ea974c4cb2d51ab4d Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Fri, 31 May 2019 17:13:32 -0400 Subject: [PATCH 16/29] reject histograms over IPs --- .../elasticsearch/index/mapper/RangeType.java | 4 +++ .../histogram/HistogramAggregatorFactory.java | 6 +++- .../RangeHistogramAggregatorTests.java | 34 ++++++++++++++++++- 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java index e819c60588603..6d396098a3c75 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java @@ -663,6 +663,10 @@ public Query rangeQuery(String field, boolean hasDocValues, Object from, Object */ public abstract Double doubleValue(Object endpointValue); + public boolean isNumeric() { + return numberType != null; + } + public abstract Query dvRangeQuery(String field, BinaryDocValuesRangeQuery.QueryType queryType, Object from, Object to, boolean includeFrom, boolean includeTo); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java index 867082fb8d5c3..26ec6358e1e84 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java @@ -22,8 +22,8 @@ import org.elasticsearch.search.aggregations.Aggregator; import org.elasticsearch.search.aggregations.AggregatorFactories; import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import org.elasticsearch.search.aggregations.BucketOrder; +import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import org.elasticsearch.search.aggregations.support.ValuesSource; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; @@ -69,6 +69,10 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, Aggregator pare return createAggregator((ValuesSource.Numeric) valuesSource, parent, pipelineAggregators, metaData); } else if (valuesSource instanceof ValuesSource.Bytes.FieldData.RangeFieldData) { + ValuesSource.Bytes.FieldData.RangeFieldData rangeValueSource = (ValuesSource.Bytes.FieldData.RangeFieldData) valuesSource; + if (rangeValueSource.rangeType().isNumeric() == false) { + throw new IllegalArgumentException("Found non-numeric range [" + rangeValueSource.rangeType().name + "]"); + } return createAggregator((ValuesSource.Bytes.FieldData.RangeFieldData) valuesSource, parent, pipelineAggregators, metaData); } else { diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java index 6b4916609d633..a08a9c29dc680 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java @@ -27,16 +27,23 @@ import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.store.Directory; import org.apache.lucene.util.BytesRef; +import org.elasticsearch.common.network.InetAddresses; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.RangeFieldMapper; import org.elasticsearch.index.mapper.RangeType; import org.elasticsearch.search.aggregations.AggregatorTestCase; import org.elasticsearch.search.aggregations.support.ValueType; +import org.junit.Rule; +import org.junit.rules.ExpectedException; import java.util.Collections; public class RangeHistogramAggregatorTests extends AggregatorTestCase { - public void testDoubleRanges() throws Exception { + + @Rule + public final ExpectedException expectedException = ExpectedException.none(); + + public void testDoubleRanges() throws Exception { RangeType rangeType = RangeType.DOUBLE; try (Directory dir = newDirectory(); @@ -58,8 +65,33 @@ public void testDoubleRanges() throws Exception { InternalHistogram histogram = search(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType); assertEquals(1, histogram.getBuckets().size()); } + } + } + + public void testIpRangesUnsupported() throws Exception { + RangeType rangeType = RangeType.IP; + try (Directory dir = newDirectory(); + RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { + Document doc = new Document(); + BytesRef encodedRange = + rangeType.encodeRanges(Collections.singleton(new RangeFieldMapper.Range(rangeType, InetAddresses.forString("10.0.0.1"), + InetAddresses.forString("10.0.0.10"), true, true))); + doc.add(new BinaryDocValuesField("field", encodedRange)); + w.addDocument(doc); + HistogramAggregationBuilder aggBuilder = new HistogramAggregationBuilder("my_agg", ValueType.RANGE) + .field("field") + .interval(5); + MappedFieldType fieldType = new RangeFieldMapper.Builder("field", rangeType).fieldType(); + fieldType.setName("field"); + + try (IndexReader reader = w.getReader()) { + IndexSearcher searcher = new IndexSearcher(reader); + expectedException.expect(IllegalArgumentException.class); + search(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType); + } } + } } From 37a50adb144ff456f736b33fece42251a4c1763a Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Mon, 3 Jun 2019 15:48:16 -0400 Subject: [PATCH 17/29] Clean up ValueType kludge from prototype phase --- .../aggregations/AggregationBuilders.java | 4 +-- .../HistogramAggregationBuilder.java | 9 +++---- .../support/ValuesSourceConfig.java | 6 ++++- .../aggregations/bucket/HistogramTests.java | 5 ++-- .../NumericHistogramAggregatorTests.java | 15 +++++------ .../RangeHistogramAggregatorTests.java | 5 ++-- .../CumulativeSumAggregatorTests.java | 7 +++-- .../pipeline/DerivativeAggregatorTests.java | 27 +++++++++---------- .../xpack/rollup/RollupRequestTranslator.java | 3 +-- .../rollup/RollupJobIdentifierUtilTests.java | 10 +++---- .../rollup/RollupRequestTranslationTests.java | 2 +- .../RollupResponseTranslationTests.java | 4 +-- 12 files changed, 47 insertions(+), 50 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java index 8fe2736d7ec74..79301c3e61826 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java @@ -89,7 +89,6 @@ import org.elasticsearch.search.aggregations.metrics.WeightedAvgAggregationBuilder; import org.elasticsearch.search.aggregations.metrics.MedianAbsoluteDeviationAggregationBuilder; import org.elasticsearch.search.aggregations.metrics.MedianAbsoluteDeviation; -import org.elasticsearch.search.aggregations.support.ValueType; import java.util.List; import java.util.Map; @@ -246,7 +245,8 @@ public static GeoDistanceAggregationBuilder geoDistance(String name, GeoPoint or * Create a new {@link Histogram} aggregation with the given name. */ public static HistogramAggregationBuilder histogram(String name) { - return new HistogramAggregationBuilder(name, ValueType.DOUBLE); + // TODO: Hardcoding to valuetype double here is probably wrong + return new HistogramAggregationBuilder(name); } /** diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java index b8408dd1afcd5..6009e7ee49a02 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java @@ -32,7 +32,6 @@ import org.elasticsearch.search.aggregations.InternalOrder; import org.elasticsearch.search.aggregations.InternalOrder.CompoundOrder; import org.elasticsearch.search.aggregations.bucket.MultiBucketAggregationBuilder; -import org.elasticsearch.search.aggregations.support.ValueType; import org.elasticsearch.search.aggregations.support.ValuesSource; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; @@ -83,7 +82,8 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder< } public static HistogramAggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException { - return PARSER.parse(parser, new HistogramAggregationBuilder(aggregationName, ValueType.DOUBLE), null); + // TODO: Fix hard coded double here? + return PARSER.parse(parser, new HistogramAggregationBuilder(aggregationName), null); } private double interval; @@ -95,8 +95,8 @@ public static HistogramAggregationBuilder parse(String aggregationName, XContent private long minDocCount = 0; /** Create a new builder with the given name. */ - public HistogramAggregationBuilder(String name, ValueType valueType) { - super(name, ValuesSourceType.ANY, valueType); + public HistogramAggregationBuilder(String name) { + super(name, ValuesSourceType.ANY, null); } protected HistogramAggregationBuilder(HistogramAggregationBuilder clone, Builder factoriesBuilder, Map metaData) { @@ -116,7 +116,6 @@ protected AggregationBuilder shallowCopy(Builder factoriesBuilder, Map ValuesSourceConfig resolve( } else if (indexFieldData instanceof IndexGeoPointFieldData) { config = new ValuesSourceConfig<>(ValuesSourceType.GEOPOINT); } else { - config = new ValuesSourceConfig<>(ValuesSourceType.BYTES); + if (fieldType instanceof RangeFieldMapper.RangeFieldType) { + config = new ValuesSourceConfig<>(ValuesSourceType.RANGE); + } else { + config = new ValuesSourceConfig<>(ValuesSourceType.BYTES); + } } } else { config = new ValuesSourceConfig<>(valueType.getValuesSourceType()); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java index ad16bd7b2f8ee..ef5cb19ed6dd7 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/HistogramTests.java @@ -22,7 +22,6 @@ import org.elasticsearch.search.aggregations.BaseAggregationTestCase; import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder; import org.elasticsearch.search.aggregations.BucketOrder; -import org.elasticsearch.search.aggregations.support.ValueType; import java.util.ArrayList; import java.util.List; @@ -34,7 +33,7 @@ public class HistogramTests extends BaseAggregationTestCase translateHistogram(HistogramAggregationB return translateVSAggBuilder(source, registry, () -> { HistogramAggregationBuilder rolledHisto - = new HistogramAggregationBuilder(source.getName(), ValueType.DOUBLE); + = new HistogramAggregationBuilder(source.getName()); rolledHisto.interval(source.interval()); rolledHisto.offset(source.offset()); diff --git a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtilTests.java b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtilTests.java index 390ab6f58cfae..555b07a0b3987 100644 --- a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtilTests.java +++ b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtilTests.java @@ -283,7 +283,7 @@ public void testComparableNoHistoVsHisto() { DateHistogramAggregationBuilder builder = new DateHistogramAggregationBuilder("foo").field("foo") .calendarInterval(new DateHistogramInterval("1h")) - .subAggregation(new HistogramAggregationBuilder("histo", ValueType.DOUBLE).field("bar").interval(100)); + .subAggregation(new HistogramAggregationBuilder("histo").field("bar").interval(100)); Set caps = new HashSet<>(2); caps.add(cap); @@ -320,7 +320,7 @@ public void testComparableNoTermsVsTerms() { } public void testHistoSameNameWrongTypeInCaps() { - HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo", ValueType.DOUBLE); + HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo"); histo.field("foo") .interval(1L) .subAggregation(new MaxAggregationBuilder("the_max").field("max_field")) @@ -406,7 +406,7 @@ public void testDateHistoMissingFieldInCaps() { } public void testHistoMissingFieldInCaps() { - HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo", ValueType.DOUBLE); + HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo"); histo.interval(1) .field("foo") .subAggregation(new MaxAggregationBuilder("the_max").field("max_field")) @@ -429,7 +429,7 @@ public void testHistoMissingFieldInCaps() { } public void testNoMatchingHistoInterval() { - HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo", ValueType.DOUBLE); + HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo"); histo.interval(1) .field("bar") .subAggregation(new MaxAggregationBuilder("the_max").field("max_field")) @@ -450,7 +450,7 @@ public void testNoMatchingHistoInterval() { } public void testHistoIntervalNotMultiple() { - HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo", ValueType.DOUBLE); + HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo"); histo.interval(10) // <--- interval is not a multiple of 3 .field("bar") .subAggregation(new MaxAggregationBuilder("the_max").field("max_field")) diff --git a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupRequestTranslationTests.java b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupRequestTranslationTests.java index 27a0b86a1a87a..27dcc751860bc 100644 --- a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupRequestTranslationTests.java +++ b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupRequestTranslationTests.java @@ -336,7 +336,7 @@ public void testStringTerms() throws IOException { public void testBasicHisto() { - HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo", ValueType.DOUBLE); + HistogramAggregationBuilder histo = new HistogramAggregationBuilder("test_histo"); histo.field("foo") .interval(1L) .extendedBounds(0.0, 1000.0) diff --git a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupResponseTranslationTests.java b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupResponseTranslationTests.java index 3d4abb12df0a4..25fe2f51b2f22 100644 --- a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupResponseTranslationTests.java +++ b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupResponseTranslationTests.java @@ -1174,10 +1174,10 @@ public void testLongTerms() throws IOException { } public void testHisto() throws IOException { - HistogramAggregationBuilder nonRollupHisto = new HistogramAggregationBuilder("histo", ValueType.DOUBLE) + HistogramAggregationBuilder nonRollupHisto = new HistogramAggregationBuilder("histo") .field("bar").interval(100); - HistogramAggregationBuilder rollupHisto = new HistogramAggregationBuilder("histo", ValueType.DOUBLE) + HistogramAggregationBuilder rollupHisto = new HistogramAggregationBuilder("histo") .field("bar.histogram." + RollupField.VALUE) .interval(100) .subAggregation(new SumAggregationBuilder("histo." + RollupField.COUNT_FIELD) From e90a192c7f64acdceb3ade27845395240f95db40 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 4 Jun 2019 11:55:33 -0400 Subject: [PATCH 18/29] Docs and small cleanup --- .../aggregations/AggregationBuilders.java | 1 - .../HistogramAggregationBuilder.java | 4 +-- .../histogram/HistogramAggregatorFactory.java | 32 ++++++++----------- 3 files changed, 15 insertions(+), 22 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java index 79301c3e61826..72ac99d94b951 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationBuilders.java @@ -245,7 +245,6 @@ public static GeoDistanceAggregationBuilder geoDistance(String name, GeoPoint or * Create a new {@link Histogram} aggregation with the given name. */ public static HistogramAggregationBuilder histogram(String name) { - // TODO: Hardcoding to valuetype double here is probably wrong return new HistogramAggregationBuilder(name); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java index 6009e7ee49a02..a62ed37f1f417 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java @@ -46,7 +46,8 @@ import java.util.Objects; /** - * A builder for histograms on numeric fields. + * A builder for histograms on numeric fields. This builder can operate on either base numeric fields, or numeric range fields. IP range + * fields are unsupported, and will throw at the factory layer. */ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder implements MultiBucketAggregationBuilder { @@ -82,7 +83,6 @@ public class HistogramAggregationBuilder extends ValuesSourceAggregationBuilder< } public static HistogramAggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException { - // TODO: Fix hard coded double here? return PARSER.parse(parser, new HistogramAggregationBuilder(aggregationName), null); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java index 26ec6358e1e84..d548733c7d855 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java @@ -33,6 +33,10 @@ import java.util.List; import java.util.Map; +/** + * Constructs the per-shard aggregator instance for histogram aggregation. Selects the numeric or range field implementation based on the + * field type. + */ public final class HistogramAggregatorFactory extends ValuesSourceAggregatorFactory { private final double interval, offset; @@ -66,14 +70,18 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, Aggregator pare return asMultiBucketAggregator(this, context, parent); } if (valuesSource instanceof ValuesSource.Numeric) { - return createAggregator((ValuesSource.Numeric) valuesSource, parent, pipelineAggregators, metaData); + return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, + (ValuesSource.Numeric) valuesSource, config.format(), context, parent, pipelineAggregators, metaData); } else if (valuesSource instanceof ValuesSource.Bytes.FieldData.RangeFieldData) { ValuesSource.Bytes.FieldData.RangeFieldData rangeValueSource = (ValuesSource.Bytes.FieldData.RangeFieldData) valuesSource; if (rangeValueSource.rangeType().isNumeric() == false) { - throw new IllegalArgumentException("Found non-numeric range [" + rangeValueSource.rangeType().name + "]"); + throw new IllegalArgumentException("Expected numeric range type but found non-numeric range [" + + rangeValueSource.rangeType().name + "]"); } - return createAggregator((ValuesSource.Bytes.FieldData.RangeFieldData) valuesSource, parent, pipelineAggregators, metaData); + return new RangeHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, + (ValuesSource.Bytes.FieldData.RangeFieldData) valuesSource, config.format(), context, parent, pipelineAggregators, + metaData); } else { throw new IllegalArgumentException("Expected one of [Numeric, RangeFieldData] values source, found [" @@ -81,24 +89,10 @@ else if (valuesSource instanceof ValuesSource.Bytes.FieldData.RangeFieldData) { } } - private Aggregator createAggregator(ValuesSource.Numeric valuesSource, Aggregator parent, List pipelineAggregators, - Map metaData) throws IOException { - - return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, - valuesSource, config.format(), context, parent, pipelineAggregators, metaData); - } - - private Aggregator createAggregator(ValuesSource.Bytes.FieldData.RangeFieldData valuesSource, Aggregator parent, - List pipelineAggregators, Map metaData) throws IOException { - - return new RangeHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, valuesSource, - config.format(), context, parent, pipelineAggregators, metaData); - } - - @Override protected Aggregator createUnmapped(Aggregator parent, List pipelineAggregators, Map metaData) throws IOException { - return createAggregator((ValuesSource.Numeric) null, parent, pipelineAggregators, metaData); + return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, + null, config.format(), context, parent, pipelineAggregators, metaData); } } From 2ddb99d051ce1a754aaeddc79cec67b5ba0156c3 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 6 Jun 2019 14:21:35 -0400 Subject: [PATCH 19/29] Fix nits --- .../org/elasticsearch/index/mapper/RangeType.java | 4 ++-- .../histogram/HistogramAggregatorFactory.java | 3 +-- .../bucket/histogram/RangeHistogramAggregator.java | 13 +++++++------ .../aggregations/support/ValuesSourceConfig.java | 9 ++++----- 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java index 6d396098a3c75..256325eba5974 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RangeType.java @@ -358,7 +358,7 @@ public List decodeRanges(BytesRef bytes) { @Override public Double doubleValue(Object endpointValue) { assert endpointValue instanceof Double; - return (Double)endpointValue; + return (Double) endpointValue; } @Override @@ -430,7 +430,7 @@ public List decodeRanges(BytesRef bytes) { } @Override - public Double doubleValue (Object endpointValue) { + public Double doubleValue(Object endpointValue) { return LONG.doubleValue(endpointValue); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java index d548733c7d855..d5e07269d9a19 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java @@ -72,8 +72,7 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, Aggregator pare if (valuesSource instanceof ValuesSource.Numeric) { return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, (ValuesSource.Numeric) valuesSource, config.format(), context, parent, pipelineAggregators, metaData); - } - else if (valuesSource instanceof ValuesSource.Bytes.FieldData.RangeFieldData) { + } else if (valuesSource instanceof ValuesSource.Bytes.FieldData.RangeFieldData) { ValuesSource.Bytes.FieldData.RangeFieldData rangeValueSource = (ValuesSource.Bytes.FieldData.RangeFieldData) valuesSource; if (rangeValueSource.rangeType().isNumeric() == false) { throw new IllegalArgumentException("Expected numeric range type but found non-numeric range [" diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java index 70ee982cff1f5..58124bc38e91a 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java @@ -59,10 +59,10 @@ public class RangeHistogramAggregator extends BucketsAggregator { private final LongHash bucketOrds; RangeHistogramAggregator(String name, AggregatorFactories factories, double interval, double offset, - BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, - @Nullable ValuesSource.Bytes.FieldData.RangeFieldData valuesSource, DocValueFormat formatter, - SearchContext context, Aggregator parent, - List pipelineAggregators, Map metaData) throws IOException { + BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, + @Nullable ValuesSource.Bytes.FieldData.RangeFieldData valuesSource, DocValueFormat formatter, + SearchContext context, Aggregator parent, + List pipelineAggregators, Map metaData) throws IOException { super(name, factories, context, parent, pipelineAggregators, metaData); if (interval <= 0) { @@ -103,8 +103,9 @@ public void collect(int doc, long bucket) throws IOException { // This list should be sorted by start-of-range, I think? List ranges = rangeType.decodeRanges(encodedRanges); for (RangeFieldMapper.Range range : ranges) { - for (double value = rangeType.doubleValue(range.getFrom()); value <= rangeType.doubleValue(range.getTo()); - value += interval) { + final Double from = rangeType.doubleValue(range.getFrom()); + final Double to = rangeType.doubleValue(range.getTo()); + for (double value = from; value <= to; value += interval) { double key = Math.floor((value - offset) / interval); if (key <= previousKey) { continue; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java index b68e622eef08b..908006b00fd42 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java @@ -101,13 +101,12 @@ public static ValuesSourceConfig resolve( config = new ValuesSourceConfig<>(ValuesSourceType.NUMERIC); } else if (indexFieldData instanceof IndexGeoPointFieldData) { config = new ValuesSourceConfig<>(ValuesSourceType.GEOPOINT); + } else if (fieldType instanceof RangeFieldMapper.RangeFieldType) { + config = new ValuesSourceConfig<>(ValuesSourceType.RANGE); } else { - if (fieldType instanceof RangeFieldMapper.RangeFieldType) { - config = new ValuesSourceConfig<>(ValuesSourceType.RANGE); - } else { - config = new ValuesSourceConfig<>(ValuesSourceType.BYTES); - } + config = new ValuesSourceConfig<>(ValuesSourceType.BYTES); } + } else { config = new ValuesSourceConfig<>(valueType.getValuesSourceType()); } From 076cdada16f13fb62960fdee6127f280b0109b73 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 6 Jun 2019 15:14:16 -0400 Subject: [PATCH 20/29] Make Range a top level value source --- .../histogram/HistogramAggregatorFactory.java | 8 ++--- .../histogram/RangeHistogramAggregator.java | 4 +-- .../aggregations/support/ValuesSource.java | 32 +++++++++++++------ .../support/ValuesSourceConfig.java | 2 +- 4 files changed, 29 insertions(+), 17 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java index d5e07269d9a19..cb96c0985a39e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java @@ -72,18 +72,18 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, Aggregator pare if (valuesSource instanceof ValuesSource.Numeric) { return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, (ValuesSource.Numeric) valuesSource, config.format(), context, parent, pipelineAggregators, metaData); - } else if (valuesSource instanceof ValuesSource.Bytes.FieldData.RangeFieldData) { - ValuesSource.Bytes.FieldData.RangeFieldData rangeValueSource = (ValuesSource.Bytes.FieldData.RangeFieldData) valuesSource; + } else if (valuesSource instanceof ValuesSource.Range) { + ValuesSource.Range rangeValueSource = (ValuesSource.Range) valuesSource; if (rangeValueSource.rangeType().isNumeric() == false) { throw new IllegalArgumentException("Expected numeric range type but found non-numeric range [" + rangeValueSource.rangeType().name + "]"); } return new RangeHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, - (ValuesSource.Bytes.FieldData.RangeFieldData) valuesSource, config.format(), context, parent, pipelineAggregators, + (ValuesSource.Range) valuesSource, config.format(), context, parent, pipelineAggregators, metaData); } else { - throw new IllegalArgumentException("Expected one of [Numeric, RangeFieldData] values source, found [" + throw new IllegalArgumentException("Expected one of [Numeric, Range] values source, found [" + valuesSource.toString() + "]"); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java index 58124bc38e91a..a17a95e88e4e8 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java @@ -48,7 +48,7 @@ import java.util.Map; public class RangeHistogramAggregator extends BucketsAggregator { - private final ValuesSource.Bytes.FieldData.RangeFieldData valuesSource; + private final ValuesSource.Range valuesSource; private final DocValueFormat formatter; private final double interval, offset; private final BucketOrder order; @@ -60,7 +60,7 @@ public class RangeHistogramAggregator extends BucketsAggregator { RangeHistogramAggregator(String name, AggregatorFactories factories, double interval, double offset, BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, - @Nullable ValuesSource.Bytes.FieldData.RangeFieldData valuesSource, DocValueFormat formatter, + @Nullable ValuesSource.Range valuesSource, DocValueFormat formatter, SearchContext context, Aggregator parent, List pipelineAggregators, Map metaData) throws IOException { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java index d722781e90a37..7582c26341a1d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java @@ -66,6 +66,28 @@ public boolean needsScores() { return false; } + public static class Range extends ValuesSource { + private final RangeType rangeType; + protected final IndexFieldData indexFieldData; + + public Range(IndexFieldData indexFieldData, RangeType rangeType) { + this.indexFieldData = indexFieldData; + this.rangeType = rangeType; + } + + @Override + public SortedBinaryDocValues bytesValues(LeafReaderContext context) { + return indexFieldData.load(context).getBytesValues(); + } + + @Override + public DocValueBits docsWithValue(LeafReaderContext context) throws IOException { + final SortedBinaryDocValues bytes = bytesValues(context); + return org.elasticsearch.index.fielddata.FieldData.docsWithValue(bytes); + } + + public RangeType rangeType() { return rangeType; } + } public abstract static class Bytes extends ValuesSource { @Override @@ -181,16 +203,6 @@ public SortedBinaryDocValues bytesValues(LeafReaderContext context) { return indexFieldData.load(context).getBytesValues(); } - public static class RangeFieldData extends FieldData { - private final RangeType rangeType; - - public RangeFieldData (IndexFieldData indexFieldData, RangeType rangeType) { - super(indexFieldData); - this.rangeType = rangeType; - } - - public RangeType rangeType() { return rangeType; } - } } public static class Script extends Bytes { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java index 908006b00fd42..98b7d2417edc4 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java @@ -361,6 +361,6 @@ private ValuesSource rangeField() { throw new IllegalStateException("Asked for range ValuesSource, but field is of type " + fieldType.name()); } RangeFieldMapper.RangeFieldType rangeFieldType = (RangeFieldMapper.RangeFieldType)fieldType; - return new ValuesSource.Bytes.FieldData.RangeFieldData(fieldContext().indexFieldData(), rangeFieldType.rangeType()); + return new ValuesSource.Range(fieldContext().indexFieldData(), rangeFieldType.rangeType()); } } From ea2f9ccbdb16a12bf16dbed30d40a242ad17549d Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 13 Jun 2019 11:10:09 -0400 Subject: [PATCH 21/29] Support for offsets in RangeHistogram --- .../histogram/RangeHistogramAggregator.java | 11 +- .../NumericHistogramAggregatorTests.java | 38 ++++ .../RangeHistogramAggregatorTests.java | 182 +++++++++++++++++- 3 files changed, 216 insertions(+), 15 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java index a17a95e88e4e8..fc3dd210fbf6e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java @@ -97,7 +97,6 @@ public void collect(int doc, long bucket) throws IOException { // values, so it isn't clear what we'd be iterating over. final int valuesCount = values.docValueCount(); - double previousKey = Double.NEGATIVE_INFINITY; for (int i = 0; i < valuesCount; i++) { BytesRef encodedRanges = values.nextValue(); // This list should be sorted by start-of-range, I think? @@ -105,11 +104,9 @@ public void collect(int doc, long bucket) throws IOException { for (RangeFieldMapper.Range range : ranges) { final Double from = rangeType.doubleValue(range.getFrom()); final Double to = rangeType.doubleValue(range.getTo()); - for (double value = from; value <= to; value += interval) { - double key = Math.floor((value - offset) / interval); - if (key <= previousKey) { - continue; - } + final double startKey = Math.floor((from - offset) / interval); + final double endKey = Math.floor((to - offset) / interval); + for (double key = startKey; key <= endKey; key++) { // Bucket collection identical to NumericHistogramAggregator, could be refactored long bucketOrd = bucketOrds.add(Double.doubleToLongBits(key)); if (bucketOrd < 0) { // already seen @@ -118,8 +115,6 @@ public void collect(int doc, long bucket) throws IOException { } else { collectBucket(sub, doc, bucketOrd); } - previousKey = key; - } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregatorTests.java index 6de6d5d6ea055..f7786358c2eb2 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregatorTests.java @@ -218,6 +218,44 @@ public void testOffset() throws Exception { } } + public void testRandomOffset() throws Exception { + try (Directory dir = newDirectory(); + RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { + // Note, these values are carefully chosen to ensure that no matter what offset we pick, no two can end up in the same bucket + for (double value : new double[] {9.3, 3.2, -5}) { + Document doc = new Document(); + doc.add(new SortedNumericDocValuesField("field", NumericUtils.doubleToSortableLong(value))); + w.addDocument(doc); + } + + final double offset = randomDouble(); + final double interval = 5; + final double expectedOffset = offset % interval; + HistogramAggregationBuilder aggBuilder = new HistogramAggregationBuilder("my_agg") + .field("field") + .interval(interval) + .offset(offset); + MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.DOUBLE); + fieldType.setName("field"); + try (IndexReader reader = w.getReader()) { + IndexSearcher searcher = new IndexSearcher(reader); + InternalHistogram histogram = search(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType); + assertEquals(3, histogram.getBuckets().size()); + + assertEquals(-10 + expectedOffset, histogram.getBuckets().get(0).getKey()); + assertEquals(1, histogram.getBuckets().get(0).getDocCount()); + + assertEquals(expectedOffset, histogram.getBuckets().get(1).getKey()); + assertEquals(1, histogram.getBuckets().get(1).getDocCount()); + + assertEquals(5 + expectedOffset, histogram.getBuckets().get(2).getKey()); + assertEquals(1, histogram.getBuckets().get(2).getDocCount()); + + assertTrue(AggregationInspectionHelper.hasValue(histogram)); + } + } + } + public void testExtendedBounds() throws Exception { try (Directory dir = newDirectory(); RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java index 0434aa99ef7e6..38693d5ef81e4 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java @@ -42,16 +42,58 @@ public class RangeHistogramAggregatorTests extends AggregatorTestCase { @Rule public final ExpectedException expectedException = ExpectedException.none(); - public void testDoubleRanges() throws Exception { + public void testMessingAround() throws Exception { + RangeType rangeType = RangeType.DOUBLE; + try (Directory dir = newDirectory(); + RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { + for (RangeFieldMapper.Range range : new RangeFieldMapper.Range[] { + new RangeFieldMapper.Range(rangeType, 4.2, 13.3, true, true), // bucket 4, 9 + }) { + Document doc = new Document(); + BytesRef encodedRange = rangeType.encodeRanges(Collections.singleton(range)); + doc.add(new BinaryDocValuesField("field", encodedRange)); + w.addDocument(doc); + } + HistogramAggregationBuilder aggBuilder = new HistogramAggregationBuilder("my_agg") + .field("field") + .interval(5) + .offset(4); + MappedFieldType fieldType = new RangeFieldMapper.Builder("field", rangeType).fieldType(); + fieldType.setName("field"); + + try (IndexReader reader = w.getReader()) { + IndexSearcher searcher = new IndexSearcher(reader); + InternalHistogram histogram = search(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType); + assertEquals(2, histogram.getBuckets().size()); + + assertEquals(4d, histogram.getBuckets().get(0).getKey()); + assertEquals(1, histogram.getBuckets().get(0).getDocCount()); + + assertEquals(9d, histogram.getBuckets().get(1).getKey()); + assertEquals(1, histogram.getBuckets().get(1).getDocCount()); + + //assertEquals(4d, histogram.getBuckets().get(2).getKey()); + //assertEquals(1, histogram.getBuckets().get(2).getDocCount()); + } + } + } + + public void testDoubleRanges() throws Exception { RangeType rangeType = RangeType.DOUBLE; try (Directory dir = newDirectory(); RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { - Document doc = new Document(); - BytesRef encodedRange = - rangeType.encodeRanges(Collections.singleton(new RangeFieldMapper.Range(rangeType, 1.0D, 3.0D, true, true))); - doc.add(new BinaryDocValuesField("field", encodedRange)); - w.addDocument(doc); + for (RangeFieldMapper.Range range : new RangeFieldMapper.Range[] { + new RangeFieldMapper.Range(rangeType, 1.0D, 5.0D, true, true), // bucket 0 5 + new RangeFieldMapper.Range(rangeType, -3.1, 4.2, true, true), // bucket -5, 0 + new RangeFieldMapper.Range(rangeType, 4.2, 13.3, true, true), // bucket 0, 5, 10 + new RangeFieldMapper.Range(rangeType, 42.5, 49.3, true, true), // bucket 40, 45 + }) { + Document doc = new Document(); + BytesRef encodedRange = rangeType.encodeRanges(Collections.singleton(range)); + doc.add(new BinaryDocValuesField("field", encodedRange)); + w.addDocument(doc); + } HistogramAggregationBuilder aggBuilder = new HistogramAggregationBuilder("my_agg") .field("field") @@ -62,11 +104,137 @@ public void testDoubleRanges() throws Exception { try (IndexReader reader = w.getReader()) { IndexSearcher searcher = new IndexSearcher(reader); InternalHistogram histogram = search(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType); - assertEquals(1, histogram.getBuckets().size()); + assertEquals(6, histogram.getBuckets().size()); + + assertEquals(-5d, histogram.getBuckets().get(0).getKey()); + assertEquals(1, histogram.getBuckets().get(0).getDocCount()); + + assertEquals(0d, histogram.getBuckets().get(1).getKey()); + assertEquals(3, histogram.getBuckets().get(1).getDocCount()); + + assertEquals(5d, histogram.getBuckets().get(2).getKey()); + assertEquals(2, histogram.getBuckets().get(2).getDocCount()); + + assertEquals(10d, histogram.getBuckets().get(3).getKey()); + assertEquals(1, histogram.getBuckets().get(3).getDocCount()); + + assertEquals(40d, histogram.getBuckets().get(4).getKey()); + assertEquals(1, histogram.getBuckets().get(4).getDocCount()); + + assertEquals(45d, histogram.getBuckets().get(5).getKey()); + assertEquals(1, histogram.getBuckets().get(5).getDocCount()); } } } + public void testOffset() throws Exception { + RangeType rangeType = RangeType.DOUBLE; + try (Directory dir = newDirectory(); + RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { + for (RangeFieldMapper.Range range : new RangeFieldMapper.Range[] { + new RangeFieldMapper.Range(rangeType, 1.0D, 5.0D, true, true), // bucket -1, 4 + new RangeFieldMapper.Range(rangeType, -3.1, 4.2, true, true), // bucket -6 -1 4 + new RangeFieldMapper.Range(rangeType, 4.2, 13.3, true, true), // bucket 4, 9 + new RangeFieldMapper.Range(rangeType, 42.5, 49.3, true, true), // bucket 39, 44, 49 + }) { + Document doc = new Document(); + BytesRef encodedRange = rangeType.encodeRanges(Collections.singleton(range)); + doc.add(new BinaryDocValuesField("field", encodedRange)); + w.addDocument(doc); + } + + HistogramAggregationBuilder aggBuilder = new HistogramAggregationBuilder("my_agg") + .field("field") + .interval(5) + .offset(4); + MappedFieldType fieldType = new RangeFieldMapper.Builder("field", rangeType).fieldType(); + fieldType.setName("field"); + + try (IndexReader reader = w.getReader()) { + IndexSearcher searcher = new IndexSearcher(reader); + InternalHistogram histogram = search(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType); + //assertEquals(7, histogram.getBuckets().size()); + + assertEquals(-6d, histogram.getBuckets().get(0).getKey()); + assertEquals(1, histogram.getBuckets().get(0).getDocCount()); + + assertEquals(-1d, histogram.getBuckets().get(1).getKey()); + assertEquals(2, histogram.getBuckets().get(1).getDocCount()); + + assertEquals(4d, histogram.getBuckets().get(2).getKey()); + assertEquals(3, histogram.getBuckets().get(2).getDocCount()); + + assertEquals(9d, histogram.getBuckets().get(3).getKey()); + assertEquals(1, histogram.getBuckets().get(3).getDocCount()); + + assertEquals(39d, histogram.getBuckets().get(4).getKey()); + assertEquals(1, histogram.getBuckets().get(4).getDocCount()); + + assertEquals(44d, histogram.getBuckets().get(5).getKey()); + assertEquals(1, histogram.getBuckets().get(5).getDocCount()); + + assertEquals(49d, histogram.getBuckets().get(6).getKey()); + assertEquals(1, histogram.getBuckets().get(6).getDocCount()); + } + } + } + + public void testOffsetGtInterval() throws Exception { + RangeType rangeType = RangeType.DOUBLE; + try (Directory dir = newDirectory(); + RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { + for (RangeFieldMapper.Range range : new RangeFieldMapper.Range[] { + new RangeFieldMapper.Range(rangeType, 1.0D, 5.0D, true, true), // bucket 0 5 + new RangeFieldMapper.Range(rangeType, -3.1, 4.2, true, true), // bucket -5, 0 + new RangeFieldMapper.Range(rangeType, 4.2, 13.3, true, true), // bucket 0, 5, 10 + new RangeFieldMapper.Range(rangeType, 42.5, 49.3, true, true), // bucket 40, 45 + }) { + Document doc = new Document(); + BytesRef encodedRange = rangeType.encodeRanges(Collections.singleton(range)); + doc.add(new BinaryDocValuesField("field", encodedRange)); + w.addDocument(doc); + } + + // I'd like to randomize the offset here, like I did in the test for the numeric side, but there's no way I can think of to + // construct the intervals such that they wouldn't "slosh" between buckets. + final double offset = 20; + final double interval = 5; + final double expectedOffset = offset % interval; + + HistogramAggregationBuilder aggBuilder = new HistogramAggregationBuilder("my_agg") + .field("field") + .interval(interval) + .offset(offset); + MappedFieldType fieldType = new RangeFieldMapper.Builder("field", rangeType).fieldType(); + fieldType.setName("field"); + + try (IndexReader reader = w.getReader()) { + IndexSearcher searcher = new IndexSearcher(reader); + InternalHistogram histogram = search(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType); + assertEquals(6, histogram.getBuckets().size()); + + assertEquals(-5d + expectedOffset, histogram.getBuckets().get(0).getKey()); + assertEquals(1, histogram.getBuckets().get(0).getDocCount()); + + assertEquals(0d + expectedOffset, histogram.getBuckets().get(1).getKey()); + assertEquals(3, histogram.getBuckets().get(1).getDocCount()); + + assertEquals(5d + expectedOffset, histogram.getBuckets().get(2).getKey()); + assertEquals(2, histogram.getBuckets().get(2).getDocCount()); + + assertEquals(10d + expectedOffset, histogram.getBuckets().get(3).getKey()); + assertEquals(1, histogram.getBuckets().get(3).getDocCount()); + + assertEquals(40d + expectedOffset, histogram.getBuckets().get(4).getKey()); + assertEquals(1, histogram.getBuckets().get(4).getDocCount()); + + assertEquals(45d + expectedOffset, histogram.getBuckets().get(5).getKey()); + assertEquals(1, histogram.getBuckets().get(5).getDocCount()); + } + } + } + + public void testIpRangesUnsupported() throws Exception { RangeType rangeType = RangeType.IP; try (Directory dir = newDirectory(); From 0b3208dbd9bfc3775e6eb1ab4cf1aed621fa3af2 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Mon, 17 Jun 2019 13:46:08 -0400 Subject: [PATCH 22/29] Test for minDocCount --- .../RangeHistogramAggregatorTests.java | 134 +++++++++++++++++- 1 file changed, 133 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java index 38693d5ef81e4..05e5c15d8547b 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java @@ -79,7 +79,7 @@ public void testMessingAround() throws Exception { } } - public void testDoubleRanges() throws Exception { + public void testDoubles() throws Exception { RangeType rangeType = RangeType.DOUBLE; try (Directory dir = newDirectory(); RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { @@ -127,6 +127,138 @@ public void testDoubleRanges() throws Exception { } } + public void testLongs() throws Exception { + RangeType rangeType = RangeType.LONG; + try (Directory dir = newDirectory(); + RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { + for (RangeFieldMapper.Range range : new RangeFieldMapper.Range[] { + new RangeFieldMapper.Range(rangeType, 1L, 5L, true, true), // bucket 0 5 + new RangeFieldMapper.Range(rangeType, -3L, 4L, true, true), // bucket -5, 0 + new RangeFieldMapper.Range(rangeType, 4L, 13L, true, true), // bucket 0, 5, 10 + new RangeFieldMapper.Range(rangeType, 42L, 49L, true, true), // bucket 40, 45 + }) { + Document doc = new Document(); + BytesRef encodedRange = rangeType.encodeRanges(Collections.singleton(range)); + doc.add(new BinaryDocValuesField("field", encodedRange)); + w.addDocument(doc); + } + + HistogramAggregationBuilder aggBuilder = new HistogramAggregationBuilder("my_agg") + .field("field") + .interval(5); + MappedFieldType fieldType = new RangeFieldMapper.Builder("field", rangeType).fieldType(); + fieldType.setName("field"); + + try (IndexReader reader = w.getReader()) { + IndexSearcher searcher = new IndexSearcher(reader); + InternalHistogram histogram = search(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType); + assertEquals(6, histogram.getBuckets().size()); + + assertEquals(-5d, histogram.getBuckets().get(0).getKey()); + assertEquals(1, histogram.getBuckets().get(0).getDocCount()); + + assertEquals(0d, histogram.getBuckets().get(1).getKey()); + assertEquals(3, histogram.getBuckets().get(1).getDocCount()); + + assertEquals(5d, histogram.getBuckets().get(2).getKey()); + assertEquals(2, histogram.getBuckets().get(2).getDocCount()); + + assertEquals(10d, histogram.getBuckets().get(3).getKey()); + assertEquals(1, histogram.getBuckets().get(3).getDocCount()); + + assertEquals(40d, histogram.getBuckets().get(4).getKey()); + assertEquals(1, histogram.getBuckets().get(4).getDocCount()); + + assertEquals(45d, histogram.getBuckets().get(5).getKey()); + assertEquals(1, histogram.getBuckets().get(5).getDocCount()); + } + } + } + + public void testLongsIrrationalInterval() throws Exception { + RangeType rangeType = RangeType.LONG; + try (Directory dir = newDirectory(); + RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { + for (RangeFieldMapper.Range range : new RangeFieldMapper.Range[] { + new RangeFieldMapper.Range(rangeType, 1L, 5L, true, true), // bucket 0 5 + new RangeFieldMapper.Range(rangeType, -3L, 4L, true, true), // bucket -5, 0 + new RangeFieldMapper.Range(rangeType, 4L, 13L, true, true), // bucket 0, 5, 10 + }) { + Document doc = new Document(); + BytesRef encodedRange = rangeType.encodeRanges(Collections.singleton(range)); + doc.add(new BinaryDocValuesField("field", encodedRange)); + w.addDocument(doc); + } + + HistogramAggregationBuilder aggBuilder = new HistogramAggregationBuilder("my_agg") + .field("field") + .interval(Math.PI); + MappedFieldType fieldType = new RangeFieldMapper.Builder("field", rangeType).fieldType(); + fieldType.setName("field"); + + try (IndexReader reader = w.getReader()) { + IndexSearcher searcher = new IndexSearcher(reader); + InternalHistogram histogram = search(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType); + assertEquals(6, histogram.getBuckets().size()); + + assertEquals(-1 * Math.PI, histogram.getBuckets().get(0).getKey()); + assertEquals(1, histogram.getBuckets().get(0).getDocCount()); + + assertEquals(0 * Math.PI, histogram.getBuckets().get(1).getKey()); + assertEquals(2, histogram.getBuckets().get(1).getDocCount()); + + assertEquals(1 * Math.PI, histogram.getBuckets().get(2).getKey()); + assertEquals(3, histogram.getBuckets().get(2).getDocCount()); + + assertEquals(2 * Math.PI, histogram.getBuckets().get(3).getKey()); + assertEquals(1, histogram.getBuckets().get(3).getDocCount()); + + assertEquals(3 * Math.PI, histogram.getBuckets().get(4).getKey()); + assertEquals(1, histogram.getBuckets().get(4).getDocCount()); + + assertEquals(4 * Math.PI, histogram.getBuckets().get(5).getKey()); + assertEquals(1, histogram.getBuckets().get(5).getDocCount()); + } + } + } + + public void testMinDocCount() throws Exception { + RangeType rangeType = RangeType.LONG; + try (Directory dir = newDirectory(); + RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { + for (RangeFieldMapper.Range range : new RangeFieldMapper.Range[] { + new RangeFieldMapper.Range(rangeType, -14L, -11L, true, true), // bucket -15 + new RangeFieldMapper.Range(rangeType, 0L, 9L, true, true), // bucket 0, 5 + new RangeFieldMapper.Range(rangeType, 6L, 12L, true, true), // bucket 5, 10 + new RangeFieldMapper.Range(rangeType, 13L, 14L, true, true), // bucket 10 + }) { + Document doc = new Document(); + BytesRef encodedRange = rangeType.encodeRanges(Collections.singleton(range)); + doc.add(new BinaryDocValuesField("field", encodedRange)); + w.addDocument(doc); + } + + HistogramAggregationBuilder aggBuilder = new HistogramAggregationBuilder("my_agg") + .field("field") + .interval(5) + .minDocCount(2); + MappedFieldType fieldType = new RangeFieldMapper.Builder("field", rangeType).fieldType(); + fieldType.setName("field"); + + try (IndexReader reader = w.getReader()) { + IndexSearcher searcher = new IndexSearcher(reader); + InternalHistogram histogram = searchAndReduce(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType); + assertEquals(2, histogram.getBuckets().size()); + + assertEquals(5d, histogram.getBuckets().get(0).getKey()); + assertEquals(2, histogram.getBuckets().get(0).getDocCount()); + + assertEquals(10d, histogram.getBuckets().get(1).getKey()); + assertEquals(2, histogram.getBuckets().get(1).getDocCount()); + } + } + } + public void testOffset() throws Exception { RangeType rangeType = RangeType.DOUBLE; try (Directory dir = newDirectory(); From 09339337e2e511179842c8f99eb96ccf5bfb89fd Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 18 Jun 2019 10:36:51 -0400 Subject: [PATCH 23/29] Support for multiple ranges on one doc --- .../histogram/RangeHistogramAggregator.java | 9 ++- .../RangeHistogramAggregatorTests.java | 65 +++++++++++-------- 2 files changed, 46 insertions(+), 28 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java index fc3dd210fbf6e..4146beb4ecb71 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java @@ -96,6 +96,7 @@ public void collect(int doc, long bucket) throws IOException { // Is it possible for valuesCount to be > 1 here? Multiple ranges are encoded into the same BytesRef in the binary doc // values, so it isn't clear what we'd be iterating over. final int valuesCount = values.docValueCount(); + double previousKey = Double.NEGATIVE_INFINITY; for (int i = 0; i < valuesCount; i++) { BytesRef encodedRanges = values.nextValue(); @@ -106,7 +107,10 @@ public void collect(int doc, long bucket) throws IOException { final Double to = rangeType.doubleValue(range.getTo()); final double startKey = Math.floor((from - offset) / interval); final double endKey = Math.floor((to - offset) / interval); - for (double key = startKey; key <= endKey; key++) { + for (double key = startKey > previousKey ? startKey : previousKey; key <= endKey; key++) { + if (key == previousKey) { + continue; + } // Bucket collection identical to NumericHistogramAggregator, could be refactored long bucketOrd = bucketOrds.add(Double.doubleToLongBits(key)); if (bucketOrd < 0) { // already seen @@ -116,6 +120,9 @@ public void collect(int doc, long bucket) throws IOException { collectBucket(sub, doc, bucketOrd); } } + if (endKey > previousKey) { + previousKey = endKey; + } } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java index 05e5c15d8547b..8821b5c9dd1a1 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregatorTests.java @@ -36,18 +36,22 @@ import org.junit.rules.ExpectedException; import java.util.Collections; +import java.util.Set; public class RangeHistogramAggregatorTests extends AggregatorTestCase { @Rule public final ExpectedException expectedException = ExpectedException.none(); - public void testMessingAround() throws Exception { + public void testDoubles() throws Exception { RangeType rangeType = RangeType.DOUBLE; try (Directory dir = newDirectory(); RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { for (RangeFieldMapper.Range range : new RangeFieldMapper.Range[] { - new RangeFieldMapper.Range(rangeType, 4.2, 13.3, true, true), // bucket 4, 9 + new RangeFieldMapper.Range(rangeType, 1.0D, 5.0D, true, true), // bucket 0 5 + new RangeFieldMapper.Range(rangeType, -3.1, 4.2, true, true), // bucket -5, 0 + new RangeFieldMapper.Range(rangeType, 4.2, 13.3, true, true), // bucket 0, 5, 10 + new RangeFieldMapper.Range(rangeType, 42.5, 49.3, true, true), // bucket 40, 45 }) { Document doc = new Document(); BytesRef encodedRange = rangeType.encodeRanges(Collections.singleton(range)); @@ -57,37 +61,45 @@ public void testMessingAround() throws Exception { HistogramAggregationBuilder aggBuilder = new HistogramAggregationBuilder("my_agg") .field("field") - .interval(5) - .offset(4); + .interval(5); MappedFieldType fieldType = new RangeFieldMapper.Builder("field", rangeType).fieldType(); fieldType.setName("field"); try (IndexReader reader = w.getReader()) { IndexSearcher searcher = new IndexSearcher(reader); InternalHistogram histogram = search(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType); - assertEquals(2, histogram.getBuckets().size()); + assertEquals(6, histogram.getBuckets().size()); - assertEquals(4d, histogram.getBuckets().get(0).getKey()); + assertEquals(-5d, histogram.getBuckets().get(0).getKey()); assertEquals(1, histogram.getBuckets().get(0).getDocCount()); - assertEquals(9d, histogram.getBuckets().get(1).getKey()); - assertEquals(1, histogram.getBuckets().get(1).getDocCount()); + assertEquals(0d, histogram.getBuckets().get(1).getKey()); + assertEquals(3, histogram.getBuckets().get(1).getDocCount()); + + assertEquals(5d, histogram.getBuckets().get(2).getKey()); + assertEquals(2, histogram.getBuckets().get(2).getDocCount()); + + assertEquals(10d, histogram.getBuckets().get(3).getKey()); + assertEquals(1, histogram.getBuckets().get(3).getDocCount()); - //assertEquals(4d, histogram.getBuckets().get(2).getKey()); - //assertEquals(1, histogram.getBuckets().get(2).getDocCount()); + assertEquals(40d, histogram.getBuckets().get(4).getKey()); + assertEquals(1, histogram.getBuckets().get(4).getDocCount()); + + assertEquals(45d, histogram.getBuckets().get(5).getKey()); + assertEquals(1, histogram.getBuckets().get(5).getDocCount()); } } } - public void testDoubles() throws Exception { - RangeType rangeType = RangeType.DOUBLE; + public void testLongs() throws Exception { + RangeType rangeType = RangeType.LONG; try (Directory dir = newDirectory(); RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { for (RangeFieldMapper.Range range : new RangeFieldMapper.Range[] { - new RangeFieldMapper.Range(rangeType, 1.0D, 5.0D, true, true), // bucket 0 5 - new RangeFieldMapper.Range(rangeType, -3.1, 4.2, true, true), // bucket -5, 0 - new RangeFieldMapper.Range(rangeType, 4.2, 13.3, true, true), // bucket 0, 5, 10 - new RangeFieldMapper.Range(rangeType, 42.5, 49.3, true, true), // bucket 40, 45 + new RangeFieldMapper.Range(rangeType, 1L, 5L, true, true), // bucket 0 5 + new RangeFieldMapper.Range(rangeType, -3L, 4L, true, true), // bucket -5, 0 + new RangeFieldMapper.Range(rangeType, 4L, 13L, true, true), // bucket 0, 5, 10 + new RangeFieldMapper.Range(rangeType, 42L, 49L, true, true), // bucket 40, 45 }) { Document doc = new Document(); BytesRef encodedRange = rangeType.encodeRanges(Collections.singleton(range)); @@ -127,21 +139,19 @@ public void testDoubles() throws Exception { } } - public void testLongs() throws Exception { + public void testMultipleRanges() throws Exception { RangeType rangeType = RangeType.LONG; try (Directory dir = newDirectory(); RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { - for (RangeFieldMapper.Range range : new RangeFieldMapper.Range[] { + Document doc = new Document(); + BytesRef encodedRange = rangeType.encodeRanges(Set.of( new RangeFieldMapper.Range(rangeType, 1L, 5L, true, true), // bucket 0 5 new RangeFieldMapper.Range(rangeType, -3L, 4L, true, true), // bucket -5, 0 new RangeFieldMapper.Range(rangeType, 4L, 13L, true, true), // bucket 0, 5, 10 - new RangeFieldMapper.Range(rangeType, 42L, 49L, true, true), // bucket 40, 45 - }) { - Document doc = new Document(); - BytesRef encodedRange = rangeType.encodeRanges(Collections.singleton(range)); - doc.add(new BinaryDocValuesField("field", encodedRange)); - w.addDocument(doc); - } + new RangeFieldMapper.Range(rangeType, 42L, 49L, true, true) // bucket 40, 45 + )); + doc.add(new BinaryDocValuesField("field", encodedRange)); + w.addDocument(doc); HistogramAggregationBuilder aggBuilder = new HistogramAggregationBuilder("my_agg") .field("field") @@ -158,10 +168,10 @@ public void testLongs() throws Exception { assertEquals(1, histogram.getBuckets().get(0).getDocCount()); assertEquals(0d, histogram.getBuckets().get(1).getKey()); - assertEquals(3, histogram.getBuckets().get(1).getDocCount()); + assertEquals(1, histogram.getBuckets().get(1).getDocCount()); assertEquals(5d, histogram.getBuckets().get(2).getKey()); - assertEquals(2, histogram.getBuckets().get(2).getDocCount()); + assertEquals(1, histogram.getBuckets().get(2).getDocCount()); assertEquals(10d, histogram.getBuckets().get(3).getKey()); assertEquals(1, histogram.getBuckets().get(3).getDocCount()); @@ -173,6 +183,7 @@ public void testLongs() throws Exception { assertEquals(1, histogram.getBuckets().get(5).getDocCount()); } } + } public void testLongsIrrationalInterval() throws Exception { From b48efdc129ebe8318913887e5b4b8f23ffe39b59 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 18 Jun 2019 16:23:15 -0400 Subject: [PATCH 24/29] ValuesSource serialization test --- .../search/aggregations/support/ValuesSourceTypeTests.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/support/ValuesSourceTypeTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/support/ValuesSourceTypeTests.java index d2f73aab3aaa3..42c276e0c4efb 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/support/ValuesSourceTypeTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/support/ValuesSourceTypeTests.java @@ -37,6 +37,7 @@ public void testValidOrdinals() { assertThat(ValuesSourceType.NUMERIC.ordinal(), equalTo(1)); assertThat(ValuesSourceType.BYTES.ordinal(), equalTo(2)); assertThat(ValuesSourceType.GEOPOINT.ordinal(), equalTo(3)); + assertThat(ValuesSourceType.RANGE.ordinal(), equalTo(4)); } @Override @@ -45,6 +46,7 @@ public void testFromString() { assertThat(ValuesSourceType.fromString("numeric"), equalTo(ValuesSourceType.NUMERIC)); assertThat(ValuesSourceType.fromString("bytes"), equalTo(ValuesSourceType.BYTES)); assertThat(ValuesSourceType.fromString("geopoint"), equalTo(ValuesSourceType.GEOPOINT)); + assertThat(ValuesSourceType.fromString("range"), equalTo(ValuesSourceType.RANGE)); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ValuesSourceType.fromString("does_not_exist")); assertThat(e.getMessage(), equalTo("No enum constant org.elasticsearch.search.aggregations.support.ValuesSourceType.DOES_NOT_EXIST")); @@ -57,6 +59,7 @@ public void testReadFrom() throws IOException { assertReadFromStream(1, ValuesSourceType.NUMERIC); assertReadFromStream(2, ValuesSourceType.BYTES); assertReadFromStream(3, ValuesSourceType.GEOPOINT); + assertReadFromStream(4, ValuesSourceType.RANGE); } @Override @@ -65,5 +68,6 @@ public void testWriteTo() throws IOException { assertWriteToStream(ValuesSourceType.NUMERIC, 1); assertWriteToStream(ValuesSourceType.BYTES, 2); assertWriteToStream(ValuesSourceType.GEOPOINT, 3); + assertWriteToStream(ValuesSourceType.RANGE, 4); } } From db9df499972faa67abea49541b13b257120bf14c Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 20 Jun 2019 14:02:09 -0400 Subject: [PATCH 25/29] Better toString() implementations for debugging missing values --- .../aggregations/support/MissingValues.java | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/MissingValues.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/MissingValues.java index d7b56af2439e0..c61091fd2a12c 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/MissingValues.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/MissingValues.java @@ -49,6 +49,11 @@ public SortedBinaryDocValues bytesValues(LeafReaderContext context) throws IOExc SortedBinaryDocValues values = valuesSource.bytesValues(context); return replaceMissing(values, missing); } + + @Override + public String toString() { + return "anon ValuesSource.Bytes of [" + super.toString() + "]"; + } }; } @@ -82,6 +87,10 @@ public BytesRef nextValue() throws IOException { return missing; } } + @Override + public String toString() { + return "anon SortedBinaryDocValues of [" + super.toString() + "]"; + } }; } @@ -111,6 +120,10 @@ public SortedNumericDoubleValues doubleValues(LeafReaderContext context) throws final SortedNumericDoubleValues values = valuesSource.doubleValues(context); return replaceMissing(values, missing.doubleValue()); } + @Override + public String toString() { + return "anon ValuesSource.Numeric of [" + super.toString() + "]"; + } }; } @@ -145,6 +158,11 @@ public boolean advanceExact(int doc) throws IOException { return true; } + @Override + public String toString() { + return "anon SortedNumericDocValues of [" + super.toString() + "]"; + } + }; } @@ -179,6 +197,11 @@ public int docValueCount() { return count == 0 ? 1 : count; } + @Override + public String toString() { + return "anon SortedNumericDoubleValues of [" + super.toString() + "]"; + } + }; } @@ -209,6 +232,12 @@ public LongUnaryOperator globalOrdinalsMapping(LeafReaderContext context) throws valuesSource.globalOrdinalsValues(context), valuesSource.globalOrdinalsMapping(context), missing); } + + @Override + public String toString() { + return "anon ValuesSource.Bytes.WithOrdinals of [" + super.toString() + "]"; + } + }; } @@ -263,6 +292,12 @@ public boolean advanceExact(int doc) throws IOException { // the document does not have a value return true; } + + @Override + public String toString() { + return "anon AbstractSortedDocValues of [" + super.toString() + "]"; + } + }; } @@ -316,6 +351,11 @@ public boolean advanceExact(int doc) throws IOException { // the document does not have a value return true; } + + @Override + public String toString() { + return "anon AbstractSortedDocValues of [" + super.toString() + "]"; + } }; } @@ -369,6 +409,11 @@ public MultiGeoPointValues geoPointValues(LeafReaderContext context) { final MultiGeoPointValues values = valuesSource.geoPointValues(context); return replaceMissing(values, missing); } + + @Override + public String toString() { + return "anon ValuesSource.GeoPoint of [" + super.toString() + "]"; + } }; } @@ -402,6 +447,11 @@ public GeoPoint nextValue() throws IOException { return missing; } } + + @Override + public String toString() { + return "anon MultiGeoPointValues of [" + super.toString() + "]"; + } }; } } From 78bce8db469d8dc9554dd534ad39d6b34379639a Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 3 Jul 2019 17:03:16 -0400 Subject: [PATCH 26/29] Fix test for unmapped missing --- .../histogram/HistogramAggregatorFactory.java | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java index cb96c0985a39e..fd40b262aff73 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java @@ -45,10 +45,19 @@ public final class HistogramAggregatorFactory extends ValuesSourceAggregatorFact private final long minDocCount; private final double minBound, maxBound; + @Override + protected ValuesSource resolveMissingAny(Object missing) { + if (missing instanceof Number) { + return ValuesSource.Numeric.EMPTY; + } + throw new IllegalArgumentException("Only numeric missing values are supported for histogram aggregation, found [" + + missing + "]"); + } + public HistogramAggregatorFactory(String name, ValuesSourceConfig config, double interval, double offset, - BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, - SearchContext context, AggregatorFactory parent, - AggregatorFactories.Builder subFactoriesBuilder, Map metaData) throws IOException { + BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, + SearchContext context, AggregatorFactory parent, + AggregatorFactories.Builder subFactoriesBuilder, Map metaData) throws IOException { super(name, config, context, parent, subFactoriesBuilder, metaData); this.interval = interval; this.offset = offset; From fa5db26415b494af8b1afec61a9300423d93869f Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 9 Jul 2019 17:19:25 -0400 Subject: [PATCH 27/29] More test fixes --- .../HistogramAggregationBuilder.java | 7 +++++ .../ValuesSourceAggregationBuilder.java | 13 ++++++++- .../support/ValuesSourceConfig.java | 27 ++++++++++++++----- .../aggregation/AggregationProfilerIT.java | 8 +++--- 4 files changed, 43 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java index e38031551fe4b..fad5c7b46b8c6 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java @@ -25,6 +25,7 @@ import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.script.Script; import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.AggregatorFactories.Builder; import org.elasticsearch.search.aggregations.AggregatorFactory; @@ -94,6 +95,12 @@ public static HistogramAggregationBuilder parse(String aggregationName, XContent private boolean keyed = false; private long minDocCount = 0; + @Override + protected ValuesSourceType resolveScriptAny(Script script) { + // TODO: No idea how we'd support Range scripts here. + return ValuesSourceType.NUMERIC; + } + /** Create a new builder with the given name. */ public HistogramAggregationBuilder(String name) { super(name, ValuesSourceType.ANY, null); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java index aad32a67a6ecf..ce5556773f1a5 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java @@ -306,10 +306,21 @@ public ZoneId timeZone() { return factory; } + /** + * Provide a hook for aggregations to have finer grained control of the ValuesSourceType for script values. This will only be called if + * the user did not supply a type hint for the script. The script object is provided for reference. + * + * @param script - The user supplied script + * @return The ValuesSourceType we expect this script to yield. + */ + protected ValuesSourceType resolveScriptAny(Script script) { + return ValuesSourceType.BYTES; + } + protected ValuesSourceConfig resolveConfig(SearchContext context) { ValueType valueType = this.valueType != null ? this.valueType : targetValueType; return ValuesSourceConfig.resolve(context.getQueryShardContext(), - valueType, field, script, missing, timeZone, format); + valueType, field, script, missing, timeZone, format, this::resolveScriptAny); } protected abstract ValuesSourceAggregatorFactory innerBuild(SearchContext context, ValuesSourceConfig config, diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java index 24eb7ef25b258..a913eca7b6eec 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java @@ -49,12 +49,25 @@ public class ValuesSourceConfig { * Resolve a {@link ValuesSourceConfig} given configuration parameters. */ public static ValuesSourceConfig resolve( - QueryShardContext context, - ValueType valueType, - String field, Script script, - Object missing, - ZoneId timeZone, - String format) { + QueryShardContext context, + ValueType valueType, + String field, Script script, + Object missing, + ZoneId timeZone, + String format) { + return resolve(context, valueType, field, script, missing, timeZone, format, s -> ValuesSourceType.BYTES); + } + + /** + * Resolve a {@link ValuesSourceConfig} given configuration parameters. + */ + public static ValuesSourceConfig resolve( + QueryShardContext context, + ValueType valueType, + String field, Script script, + Object missing, + ZoneId timeZone, + String format, Function resolveScriptAny) { if (field == null) { if (script == null) { @@ -68,7 +81,7 @@ public static ValuesSourceConfig resolve( // we need to have a specific value source // type to know how to handle the script values, so we fallback // on Bytes - valuesSourceType = ValuesSourceType.BYTES; + valuesSourceType = resolveScriptAny.apply(script); } ValuesSourceConfig config = new ValuesSourceConfig<>(valuesSourceType); config.missing(missing); diff --git a/server/src/test/java/org/elasticsearch/search/profile/aggregation/AggregationProfilerIT.java b/server/src/test/java/org/elasticsearch/search/profile/aggregation/AggregationProfilerIT.java index 92bf4d6acad2c..197e82ea3a47b 100644 --- a/server/src/test/java/org/elasticsearch/search/profile/aggregation/AggregationProfilerIT.java +++ b/server/src/test/java/org/elasticsearch/search/profile/aggregation/AggregationProfilerIT.java @@ -100,7 +100,7 @@ public void testSimpleProfile() { ProfileResult histoAggResult = aggProfileResultsList.get(0); assertThat(histoAggResult, notNullValue()); assertThat(histoAggResult.getQueryName(), - equalTo("HistogramAggregator")); + equalTo("NumericHistogramAggregator")); assertThat(histoAggResult.getLuceneDescription(), equalTo("histo")); assertThat(histoAggResult.getProfiledChildren().size(), equalTo(0)); assertThat(histoAggResult.getTime(), greaterThan(0L)); @@ -145,7 +145,7 @@ public void testMultiLevelProfile() { ProfileResult histoAggResult = aggProfileResultsList.get(0); assertThat(histoAggResult, notNullValue()); assertThat(histoAggResult.getQueryName(), - equalTo("HistogramAggregator")); + equalTo("NumericHistogramAggregator")); assertThat(histoAggResult.getLuceneDescription(), equalTo("histo")); assertThat(histoAggResult.getTime(), greaterThan(0L)); Map histoBreakdown = histoAggResult.getTimeBreakdown(); @@ -215,7 +215,7 @@ public void testMultiLevelProfileBreadthFirst() { ProfileResult histoAggResult = aggProfileResultsList.get(0); assertThat(histoAggResult, notNullValue()); assertThat(histoAggResult.getQueryName(), - equalTo("HistogramAggregator")); + equalTo("NumericHistogramAggregator")); assertThat(histoAggResult.getLuceneDescription(), equalTo("histo")); assertThat(histoAggResult.getTime(), greaterThan(0L)); Map histoBreakdown = histoAggResult.getTimeBreakdown(); @@ -346,7 +346,7 @@ public void testComplexProfile() { ProfileResult histoAggResult = aggProfileResultsList.get(0); assertThat(histoAggResult, notNullValue()); assertThat(histoAggResult.getQueryName(), - equalTo("HistogramAggregator")); + equalTo("NumericHistogramAggregator")); assertThat(histoAggResult.getLuceneDescription(), equalTo("histo")); assertThat(histoAggResult.getTime(), greaterThan(0L)); Map histoBreakdown = histoAggResult.getTimeBreakdown(); From ac3e717697f6bea00e40f0f250fd4b4f1afd4051 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 11 Jul 2019 12:36:16 -0400 Subject: [PATCH 28/29] Fix BCW serialization issue --- .../HistogramAggregationBuilder.java | 6 +++-- .../missing/MissingAggregationBuilder.java | 3 ++- .../SignificantTermsAggregationBuilder.java | 3 ++- .../terms/RareTermsAggregationBuilder.java | 3 ++- .../bucket/terms/TermsAggregationBuilder.java | 3 ++- .../CardinalityAggregationBuilder.java | 3 ++- .../metrics/ValueCountAggregationBuilder.java | 3 ++- .../ValuesSourceAggregationBuilder.java | 25 +++++++++++++------ 8 files changed, 33 insertions(+), 16 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java index b628e08c793c0..f990bded291ae 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregationBuilder.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.bucket.histogram; +import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -135,8 +136,9 @@ public HistogramAggregationBuilder(StreamInput in) throws IOException { } @Override - protected boolean serializeTargetValueType() { - return true; + protected boolean serializeTargetValueType(Version version) { + // TODO: Update version number after backport + return version.onOrAfter(Version.V_8_0_0); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregationBuilder.java index de729b619dcd4..c0fd5f26eb559 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregationBuilder.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.bucket.missing; +import org.elasticsearch.Version; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ObjectParser; @@ -78,7 +79,7 @@ protected void innerWriteTo(StreamOutput out) { } @Override - protected boolean serializeTargetValueType() { + protected boolean serializeTargetValueType(Version version) { return true; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregationBuilder.java index 75b32f8abe062..dab9cf34dbb7c 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/SignificantTermsAggregationBuilder.java @@ -18,6 +18,7 @@ */ package org.elasticsearch.search.aggregations.bucket.significant; +import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -155,7 +156,7 @@ protected void innerWriteTo(StreamOutput out) throws IOException { } @Override - protected boolean serializeTargetValueType() { + protected boolean serializeTargetValueType(Version version) { return true; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/RareTermsAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/RareTermsAggregationBuilder.java index 285869dd2e0cf..f22eaf4d28a59 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/RareTermsAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/RareTermsAggregationBuilder.java @@ -18,6 +18,7 @@ */ package org.elasticsearch.search.aggregations.bucket.terms; +import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -94,7 +95,7 @@ public RareTermsAggregationBuilder(StreamInput in) throws IOException { } @Override - protected boolean serializeTargetValueType() { + protected boolean serializeTargetValueType(Version version) { return true; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java index a124feb115b19..7d5bda9ef1b81 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregationBuilder.java @@ -18,6 +18,7 @@ */ package org.elasticsearch.search.aggregations.bucket.terms; +import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -140,7 +141,7 @@ public TermsAggregationBuilder(StreamInput in) throws IOException { } @Override - protected boolean serializeTargetValueType() { + protected boolean serializeTargetValueType(Version version) { return true; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregationBuilder.java index 8d927e2fa59eb..0cc2b7d09c0c2 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregationBuilder.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.metrics; +import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -96,7 +97,7 @@ protected void innerWriteTo(StreamOutput out) throws IOException { } @Override - protected boolean serializeTargetValueType() { + protected boolean serializeTargetValueType(Version version) { return true; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationBuilder.java index ccf8ef8ba3dca..845fab414a3ac 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationBuilder.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.metrics; +import org.elasticsearch.Version; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ObjectParser; @@ -78,7 +79,7 @@ protected void innerWriteTo(StreamOutput out) { } @Override - protected boolean serializeTargetValueType() { + protected boolean serializeTargetValueType(Version version) { return true; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java index 4835d12d12f57..d1d72313688e4 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java @@ -18,6 +18,7 @@ */ package org.elasticsearch.search.aggregations.support; +import org.elasticsearch.Version; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -61,7 +62,7 @@ protected LeafOnly(StreamInput in, ValuesSourceType valuesSourceType, ValueType /** * Read an aggregation from a stream that serializes its targetValueType. This should only be used by subclasses that override - * {@link #serializeTargetValueType()} to return true. + * {@link #serializeTargetValueType(Version)} to return true. */ protected LeafOnly(StreamInput in, ValuesSourceType valuesSourceType) throws IOException { super(in, valuesSourceType); @@ -108,24 +109,31 @@ protected ValuesSourceAggregationBuilder(ValuesSourceAggregationBuilder } /** - * Read an aggregation from a stream that does not serialize its targetValueType. This should be used by most subclasses. + * Read an aggregation from a stream that has a sensible default for TargetValueType. This should be used by most subclasses. + * Subclasses needing to maintain backward compatibility to a version that did not serialize TargetValueType should use this + * constructor, providing the old, constant value for TargetValueType and override {@link #serializeTargetValueType(Version)} to return + * true only for versions that support the serialization. */ protected ValuesSourceAggregationBuilder(StreamInput in, ValuesSourceType valuesSourceType, ValueType targetValueType) throws IOException { super(in); - assert false == serializeTargetValueType() : "Wrong read constructor called for subclass that provides its targetValueType"; this.valuesSourceType = valuesSourceType; - this.targetValueType = targetValueType; + if (serializeTargetValueType(in.getVersion())) { + this.targetValueType = in.readOptionalWriteable(ValueType::readFromStream); + } else { + this.targetValueType = targetValueType; + } read(in); } /** * Read an aggregation from a stream that serializes its targetValueType. This should only be used by subclasses that override - * {@link #serializeTargetValueType()} to return true. + * {@link #serializeTargetValueType(Version)} to return true. */ protected ValuesSourceAggregationBuilder(StreamInput in, ValuesSourceType valuesSourceType) throws IOException { super(in); - assert serializeTargetValueType() : "Wrong read constructor called for subclass that serializes its targetValueType"; + // TODO: Can we get rid of this constructor and always use the three value version? Does this assert provide any value? + assert serializeTargetValueType(in.getVersion()) : "Wrong read constructor called for subclass that serializes its targetValueType"; this.valuesSourceType = valuesSourceType; this.targetValueType = in.readOptionalWriteable(ValueType::readFromStream); read(in); @@ -149,7 +157,7 @@ private void read(StreamInput in) throws IOException { @Override protected final void doWriteTo(StreamOutput out) throws IOException { - if (serializeTargetValueType()) { + if (serializeTargetValueType(out.getVersion())) { out.writeOptionalWriteable(targetValueType); } out.writeOptionalString(field); @@ -177,8 +185,9 @@ protected final void doWriteTo(StreamOutput out) throws IOException { /** * Should this builder serialize its targetValueType? Defaults to false. All subclasses that override this to true should use the three * argument read constructor rather than the four argument version. + * @param version For backwards compatibility, subclasses can change behavior based on the version */ - protected boolean serializeTargetValueType() { + protected boolean serializeTargetValueType(Version version) { return false; } From 4da53b88b23b40a5f216510d7bea3d2837a951d6 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Mon, 15 Jul 2019 16:27:04 -0400 Subject: [PATCH 29/29] Response to PR feedback --- .../histogram/RangeHistogramAggregator.java | 5 ++- .../support/ValuesSourceConfig.java | 2 - .../RangeHistogramAggregatorTests.java | 38 +++++++++++++++++++ 3 files changed, 42 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java index 4146beb4ecb71..1a722dc951418 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java @@ -96,14 +96,17 @@ public void collect(int doc, long bucket) throws IOException { // Is it possible for valuesCount to be > 1 here? Multiple ranges are encoded into the same BytesRef in the binary doc // values, so it isn't clear what we'd be iterating over. final int valuesCount = values.docValueCount(); + assert valuesCount == 1 : "Value count for ranges should always be 1"; double previousKey = Double.NEGATIVE_INFINITY; for (int i = 0; i < valuesCount; i++) { BytesRef encodedRanges = values.nextValue(); - // This list should be sorted by start-of-range, I think? List ranges = rangeType.decodeRanges(encodedRanges); + double previousFrom = Double.NEGATIVE_INFINITY; for (RangeFieldMapper.Range range : ranges) { final Double from = rangeType.doubleValue(range.getFrom()); + // The encoding should ensure that this assert is always true. + assert from >= previousFrom : "Start of range not >= previous start"; final Double to = rangeType.doubleValue(range.getTo()); final double startKey = Math.floor((from - offset) / interval); final double endKey = Math.floor((to - offset) / interval); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java index a913eca7b6eec..a9e59519dcf6c 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java @@ -269,7 +269,6 @@ public VS toValuesSource(QueryShardContext context, Function