From 205918956277fd68bffad9959e788be783609f9b Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 28 Jan 2020 16:10:42 -0500 Subject: [PATCH 1/5] small cleanups --- .../aggregations/support/ValueType.java | 33 ++++++++----------- .../support/ValuesSourceConfig.java | 2 +- 2 files changed, 14 insertions(+), 21 deletions(-) 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 bfe642caaad20..0605039c8baa2 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 @@ -23,10 +23,6 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; -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; @@ -36,24 +32,23 @@ public enum ValueType implements Writeable { STRING((byte) 1, "string", "string", CoreValuesSourceType.BYTES, - IndexFieldData.class, DocValueFormat.RAW), + DocValueFormat.RAW), - LONG((byte) 2, "byte|short|integer|long", "long", CoreValuesSourceType.NUMERIC, IndexNumericFieldData.class, DocValueFormat.RAW), - DOUBLE((byte) 3, "float|double", "double", CoreValuesSourceType.NUMERIC, IndexNumericFieldData.class, DocValueFormat.RAW), - NUMBER((byte) 4, "number", "number", CoreValuesSourceType.NUMERIC, IndexNumericFieldData.class, DocValueFormat.RAW), - DATE((byte) 5, "date", "date", CoreValuesSourceType.DATE, IndexNumericFieldData.class, - new DocValueFormat.DateTime(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER, ZoneOffset.UTC, + LONG((byte) 2, "byte|short|integer|long", "long", CoreValuesSourceType.NUMERIC, DocValueFormat.RAW), + DOUBLE((byte) 3, "float|double", "double", CoreValuesSourceType.NUMERIC, DocValueFormat.RAW), + NUMBER((byte) 4, "number", "number", CoreValuesSourceType.NUMERIC, DocValueFormat.RAW), + DATE((byte) 5, "date", "date", CoreValuesSourceType.DATE, + new DocValueFormat.DateTime(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER, ZoneOffset.UTC, DateFieldMapper.Resolution.MILLISECONDS)), - IP((byte) 6, "ip", "ip", CoreValuesSourceType.IP, IndexFieldData.class, DocValueFormat.IP), + IP((byte) 6, "ip", "ip", CoreValuesSourceType.IP, DocValueFormat.IP), // TODO: what is the difference between "number" and "numeric"? - NUMERIC((byte) 7, "numeric", "numeric", CoreValuesSourceType.NUMERIC, IndexNumericFieldData.class, DocValueFormat.RAW), - GEOPOINT((byte) 8, "geo_point", "geo_point", CoreValuesSourceType.GEOPOINT, IndexGeoPointFieldData.class, DocValueFormat.GEOHASH), - BOOLEAN((byte) 9, "boolean", "boolean", CoreValuesSourceType.BOOLEAN, IndexNumericFieldData.class, DocValueFormat.BOOLEAN), - RANGE((byte) 10, "range", "range", CoreValuesSourceType.RANGE, BinaryDVIndexFieldData.class, DocValueFormat.RAW); + NUMERIC((byte) 7, "numeric", "numeric", CoreValuesSourceType.NUMERIC, DocValueFormat.RAW), + GEOPOINT((byte) 8, "geo_point", "geo_point", CoreValuesSourceType.GEOPOINT, DocValueFormat.GEOHASH), + BOOLEAN((byte) 9, "boolean", "boolean", CoreValuesSourceType.BOOLEAN, DocValueFormat.BOOLEAN), + RANGE((byte) 10, "range", "range", CoreValuesSourceType.RANGE, DocValueFormat.RAW); final String description; final ValuesSourceType valuesSourceType; - final Class fieldDataType; final DocValueFormat defaultFormat; private final byte id; private String preferredName; @@ -61,12 +56,11 @@ public enum ValueType implements Writeable { public static final ParseField VALUE_TYPE = new ParseField("value_type", "valueType"); ValueType(byte id, String description, String preferredName, ValuesSourceType valuesSourceType, - Class fieldDataType, DocValueFormat defaultFormat) { + DocValueFormat defaultFormat) { this.id = id; this.description = description; this.preferredName = preferredName; this.valuesSourceType = valuesSourceType; - this.fieldDataType = fieldDataType; this.defaultFormat = defaultFormat; } @@ -79,8 +73,7 @@ public ValuesSourceType getValuesSourceType() { } public boolean isA(ValueType valueType) { - return valueType.valuesSourceType.isCastableTo(valuesSourceType) && - valueType.fieldDataType.isAssignableFrom(fieldDataType); + return valueType.valuesSourceType.isCastableTo(valuesSourceType); } public boolean isNotA(ValueType valueType) { 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 e1a4fec8adb96..100e1d912f9d8 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 @@ -195,7 +195,7 @@ public ValuesSourceConfig script(AggregationScript.LeafFactory script) { return this; } - public ValuesSourceConfig scriptValueType(ValueType scriptValueType) { + private ValuesSourceConfig scriptValueType(ValueType scriptValueType) { this.scriptValueType = scriptValueType; return this; } From b94691f838543bf034a1a93264f9bd3ea51ccff1 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 28 Jan 2020 16:11:09 -0500 Subject: [PATCH 2/5] Remove generic values source from MultiValuesSource* --- .../WeightedAvgAggregationBuilder.java | 13 +++++------ .../metrics/WeightedAvgAggregatorFactory.java | 3 +-- .../MultiValuesSourceAggregationBuilder.java | 22 +++++++++---------- .../MultiValuesSourceAggregatorFactory.java | 3 +-- .../support/MultiValuesSourceParseHelper.java | 6 ++--- 5 files changed, 22 insertions(+), 25 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/WeightedAvgAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/WeightedAvgAggregationBuilder.java index ee8b77fa25f70..1fb9cbfa12992 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/WeightedAvgAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/WeightedAvgAggregationBuilder.java @@ -36,14 +36,13 @@ import org.elasticsearch.search.aggregations.support.MultiValuesSourceFieldConfig; import org.elasticsearch.search.aggregations.support.MultiValuesSourceParseHelper; import org.elasticsearch.search.aggregations.support.ValueType; -import org.elasticsearch.search.aggregations.support.ValuesSource.Numeric; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; import java.io.IOException; import java.util.Map; import java.util.Objects; -public class WeightedAvgAggregationBuilder extends MultiValuesSourceAggregationBuilder.LeafOnly { +public class WeightedAvgAggregationBuilder extends MultiValuesSourceAggregationBuilder.LeafOnly { public static final String NAME = "weighted_avg"; public static final ParseField VALUE_FIELD = new ParseField("value"); public static final ParseField WEIGHT_FIELD = new ParseField("weight"); @@ -98,11 +97,11 @@ protected void innerWriteTo(StreamOutput out) { } @Override - protected MultiValuesSourceAggregatorFactory innerBuild(QueryShardContext queryShardContext, - Map configs, - DocValueFormat format, - AggregatorFactory parent, - Builder subFactoriesBuilder) throws IOException { + protected MultiValuesSourceAggregatorFactory innerBuild(QueryShardContext queryShardContext, + Map configs, + DocValueFormat format, + AggregatorFactory parent, + Builder subFactoriesBuilder) throws IOException { return new WeightedAvgAggregatorFactory(name, configs, format, queryShardContext, parent, subFactoriesBuilder, metaData); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/WeightedAvgAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/WeightedAvgAggregatorFactory.java index fecfabba98252..a9fedbf0e7d3e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/WeightedAvgAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/WeightedAvgAggregatorFactory.java @@ -27,7 +27,6 @@ import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import org.elasticsearch.search.aggregations.support.MultiValuesSource; import org.elasticsearch.search.aggregations.support.MultiValuesSourceAggregatorFactory; -import org.elasticsearch.search.aggregations.support.ValuesSource.Numeric; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; import org.elasticsearch.search.internal.SearchContext; @@ -35,7 +34,7 @@ import java.util.List; import java.util.Map; -class WeightedAvgAggregatorFactory extends MultiValuesSourceAggregatorFactory { +class WeightedAvgAggregatorFactory extends MultiValuesSourceAggregatorFactory { WeightedAvgAggregatorFactory(String name, Map configs, DocValueFormat format, QueryShardContext queryShardContext, AggregatorFactory parent, diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceAggregationBuilder.java index 937b3e77d743a..b89d937f7dde3 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceAggregationBuilder.java @@ -40,18 +40,18 @@ * * A limitation of this class is that all the ValuesSource's being refereenced must be of the same type. */ -public abstract class MultiValuesSourceAggregationBuilder> +public abstract class MultiValuesSourceAggregationBuilder> extends AbstractAggregationBuilder { - public abstract static class LeafOnly> - extends MultiValuesSourceAggregationBuilder { + public abstract static class LeafOnly> + extends MultiValuesSourceAggregationBuilder { protected LeafOnly(String name, ValueType targetValueType) { super(name, targetValueType); } - protected LeafOnly(LeafOnly clone, Builder factoriesBuilder, Map metaData) { + protected LeafOnly(LeafOnly clone, Builder factoriesBuilder, Map metaData) { super(clone, factoriesBuilder, metaData); if (factoriesBuilder.count() > 0) { throw new AggregationInitializationException("Aggregator [" + name + "] of type [" @@ -85,7 +85,7 @@ protected MultiValuesSourceAggregationBuilder(String name, ValueType targetValue this.targetValueType = targetValueType; } - protected MultiValuesSourceAggregationBuilder(MultiValuesSourceAggregationBuilder clone, + protected MultiValuesSourceAggregationBuilder(MultiValuesSourceAggregationBuilder clone, Builder factoriesBuilder, Map metaData) { super(clone, factoriesBuilder, metaData); @@ -163,8 +163,8 @@ public AB format(String format) { } @Override - protected final MultiValuesSourceAggregatorFactory doBuild(QueryShardContext queryShardContext, AggregatorFactory parent, - Builder subFactoriesBuilder) throws IOException { + protected final MultiValuesSourceAggregatorFactory doBuild(QueryShardContext queryShardContext, AggregatorFactory parent, + Builder subFactoriesBuilder) throws IOException { ValueType finalValueType = this.valueType != null ? this.valueType : targetValueType; Map configs = new HashMap<>(fields.size()); @@ -189,10 +189,10 @@ private static DocValueFormat resolveFormat(@Nullable String format, @Nullable V return valueFormat; } - protected abstract MultiValuesSourceAggregatorFactory innerBuild(QueryShardContext queryShardContext, - Map configs, - DocValueFormat format, AggregatorFactory parent, - Builder subFactoriesBuilder) throws IOException; + protected abstract MultiValuesSourceAggregatorFactory innerBuild(QueryShardContext queryShardContext, + Map configs, + DocValueFormat format, AggregatorFactory parent, + Builder subFactoriesBuilder) throws IOException; /** diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceAggregatorFactory.java index e78029967862f..711c53f13d9df 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceAggregatorFactory.java @@ -31,8 +31,7 @@ import java.util.List; import java.util.Map; -public abstract class MultiValuesSourceAggregatorFactory - extends AggregatorFactory { +public abstract class MultiValuesSourceAggregatorFactory extends AggregatorFactory { protected final Map configs; protected final DocValueFormat format; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceParseHelper.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceParseHelper.java index dc87efa7edbc8..76f3327d77738 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceParseHelper.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceParseHelper.java @@ -28,8 +28,8 @@ public final class MultiValuesSourceParseHelper { public static void declareCommon( - AbstractObjectParser, T> objectParser, boolean formattable, - ValueType expectedValueType) { + AbstractObjectParser, T> objectParser, boolean formattable, + ValueType expectedValueType) { objectParser.declareField(MultiValuesSourceAggregationBuilder::valueType, p -> { ValueType valueType = ValueType.resolveForScript(p.text()); @@ -49,7 +49,7 @@ public static void declareCommon( } public static void declareField(String fieldName, - AbstractObjectParser, T> objectParser, + AbstractObjectParser, T> objectParser, boolean scriptable, boolean timezoneAware) { objectParser.declareField((o, fieldConfig) -> o.field(fieldName, fieldConfig.build()), From b16b929e8c11f41b27b67c43dad1e8ed45499d5a Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 29 Jan 2020 13:30:22 -0500 Subject: [PATCH 3/5] Remove targetValueType from MultiValuesSourceAggregationBuilder --- .../ArrayValuesSourceAggregationBuilder.java | 2 +- .../inject/multibindings/MapBinder.java | 12 +-- .../common/inject/util/Types.java | 4 +- .../CompositeValuesSourceBuilder.java | 2 +- .../WeightedAvgAggregationBuilder.java | 11 ++- .../MultiValuesSourceAggregationBuilder.java | 76 +++++++++---------- .../support/MultiValuesSourceParseHelper.java | 2 +- .../aggregations/support/ValueType.java | 4 +- .../ValuesSourceAggregationBuilder.java | 6 +- 9 files changed, 61 insertions(+), 58 deletions(-) diff --git a/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregationBuilder.java b/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregationBuilder.java index 3d86f71077eee..3c4554e0ed112 100644 --- a/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregationBuilder.java +++ b/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregationBuilder.java @@ -72,7 +72,7 @@ public AB subAggregations(Builder subFactories) { } private List fields = Collections.emptyList(); - /* The parser doesn't support setting userValueTypeHint (aka valueType), but we do serialize and deserialize it, so keeping it around + /* The parser doesn't support setting userValueTypeHint (aka userValueTypeHint), but we do serialize and deserialize it, so keeping it around for now so as to not break BWC. Future refactors should feel free to remove this field. --Tozzi 2020-01-16 */ private ValueType userValueTypeHint = null; diff --git a/server/src/main/java/org/elasticsearch/common/inject/multibindings/MapBinder.java b/server/src/main/java/org/elasticsearch/common/inject/multibindings/MapBinder.java index a0a22d96f58d5..f7458c888a0e6 100644 --- a/server/src/main/java/org/elasticsearch/common/inject/multibindings/MapBinder.java +++ b/server/src/main/java/org/elasticsearch/common/inject/multibindings/MapBinder.java @@ -99,7 +99,7 @@ private MapBinder() { } /** - * Returns a new mapbinder that collects entries of {@code keyType}/{@code valueType} in a + * Returns a new mapbinder that collects entries of {@code keyType}/{@code userValueTypeHint} in a * {@link Map} that is itself bound with no binding annotation. */ public static MapBinder newMapBinder(Binder binder, @@ -112,7 +112,7 @@ public static MapBinder newMapBinder(Binder binder, } /** - * Returns a new mapbinder that collects entries of {@code keyType}/{@code valueType} in a + * Returns a new mapbinder that collects entries of {@code keyType}/{@code userValueTypeHint} in a * {@link Map} that is itself bound with no binding annotation. */ public static MapBinder newMapBinder(Binder binder, @@ -121,7 +121,7 @@ public static MapBinder newMapBinder(Binder binder, } /** - * Returns a new mapbinder that collects entries of {@code keyType}/{@code valueType} in a + * Returns a new mapbinder that collects entries of {@code keyType}/{@code userValueTypeHint} in a * {@link Map} that is itself bound with {@code annotation}. */ public static MapBinder newMapBinder(Binder binder, @@ -134,7 +134,7 @@ public static MapBinder newMapBinder(Binder binder, } /** - * Returns a new mapbinder that collects entries of {@code keyType}/{@code valueType} in a + * Returns a new mapbinder that collects entries of {@code keyType}/{@code userValueTypeHint} in a * {@link Map} that is itself bound with {@code annotation}. */ public static MapBinder newMapBinder(Binder binder, @@ -143,7 +143,7 @@ public static MapBinder newMapBinder(Binder binder, } /** - * Returns a new mapbinder that collects entries of {@code keyType}/{@code valueType} in a + * Returns a new mapbinder that collects entries of {@code keyType}/{@code userValueTypeHint} in a * {@link Map} that is itself bound with {@code annotationType}. */ public static MapBinder newMapBinder(Binder binder, TypeLiteral keyType, @@ -156,7 +156,7 @@ public static MapBinder newMapBinder(Binder binder, TypeLiteral } /** - * Returns a new mapbinder that collects entries of {@code keyType}/{@code valueType} in a + * Returns a new mapbinder that collects entries of {@code keyType}/{@code userValueTypeHint} in a * {@link Map} that is itself bound with {@code annotationType}. */ public static MapBinder newMapBinder(Binder binder, Class keyType, diff --git a/server/src/main/java/org/elasticsearch/common/inject/util/Types.java b/server/src/main/java/org/elasticsearch/common/inject/util/Types.java index a3f42a0d374cc..05990502d948c 100644 --- a/server/src/main/java/org/elasticsearch/common/inject/util/Types.java +++ b/server/src/main/java/org/elasticsearch/common/inject/util/Types.java @@ -113,7 +113,7 @@ public static ParameterizedType setOf(Type elementType) { /** * Returns a type modelling a {@link Map} whose keys are of type - * {@code keyType} and whose values are of type {@code valueType}. + * {@code keyType} and whose values are of type {@code userValueTypeHint}. * * @return a parameterized type. */ @@ -132,4 +132,4 @@ public static ParameterizedType mapOf(Type keyType, Type valueType) { public static ParameterizedType providerOf(Type providedType) { return newParameterizedType(Provider.class, providedType); } -} \ No newline at end of file +} diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java index 8507fad49bfd2..04ec2c6d1460f 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java @@ -186,7 +186,7 @@ public Script script() { @SuppressWarnings("unchecked") public AB valueType(ValueType valueType) { if (valueType == null) { - throw new IllegalArgumentException("[valueType] must not be null"); + throw new IllegalArgumentException("[userValueTypeHint] must not be null"); } this.valueType = valueType; return (AB) this; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/WeightedAvgAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/WeightedAvgAggregationBuilder.java index 1fb9cbfa12992..ddac2e1b6dae9 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/WeightedAvgAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/WeightedAvgAggregationBuilder.java @@ -31,12 +31,14 @@ import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.AggregatorFactories.Builder; import org.elasticsearch.search.aggregations.AggregatorFactory; +import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; import org.elasticsearch.search.aggregations.support.MultiValuesSourceAggregationBuilder; import org.elasticsearch.search.aggregations.support.MultiValuesSourceAggregatorFactory; import org.elasticsearch.search.aggregations.support.MultiValuesSourceFieldConfig; import org.elasticsearch.search.aggregations.support.MultiValuesSourceParseHelper; import org.elasticsearch.search.aggregations.support.ValueType; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; +import org.elasticsearch.search.aggregations.support.ValuesSourceType; import java.io.IOException; import java.util.Map; @@ -60,7 +62,7 @@ public static AggregationBuilder parse(String aggregationName, XContentParser pa } public WeightedAvgAggregationBuilder(String name) { - super(name, ValueType.NUMERIC); + super(name); } public WeightedAvgAggregationBuilder(WeightedAvgAggregationBuilder clone, Builder factoriesBuilder, Map metaData) { @@ -83,7 +85,7 @@ public WeightedAvgAggregationBuilder weight(MultiValuesSourceFieldConfig weightC * Read from a stream. */ public WeightedAvgAggregationBuilder(StreamInput in) throws IOException { - super(in, ValueType.NUMERIC); + super(in); } @Override @@ -91,6 +93,11 @@ protected AggregationBuilder shallowCopy(Builder factoriesBuilder, Map> extends MultiValuesSourceAggregationBuilder { - protected LeafOnly(String name, ValueType targetValueType) { - super(name, targetValueType); + protected LeafOnly(String name) { + super(name); } protected LeafOnly(LeafOnly clone, Builder factoriesBuilder, Map metaData) { @@ -62,8 +62,8 @@ protected LeafOnly(LeafOnly clone, Builder factoriesBuilder, Map fields = new HashMap<>(); - private final ValueType targetValueType; - private ValueType valueType = null; + private ValueType userValueTypeHint = null; private String format = null; - protected MultiValuesSourceAggregationBuilder(String name, ValueType targetValueType) { + protected MultiValuesSourceAggregationBuilder(String name) { super(name); - this.targetValueType = targetValueType; } protected MultiValuesSourceAggregationBuilder(MultiValuesSourceAggregationBuilder clone, @@ -90,16 +88,16 @@ protected MultiValuesSourceAggregationBuilder(MultiValuesSourceAggregationBuilde super(clone, factoriesBuilder, metaData); this.fields = new HashMap<>(clone.fields); - this.targetValueType = clone.targetValueType; - this.valueType = clone.valueType; + this.userValueTypeHint = clone.userValueTypeHint; this.format = clone.format; } - protected MultiValuesSourceAggregationBuilder(StreamInput in, ValueType targetValueType) + /** + * Read from a stream. + */ + protected MultiValuesSourceAggregationBuilder(StreamInput in) throws IOException { super(in); - assert false == serializeTargetValueType() : "Wrong read constructor called for subclass that provides its targetValueType"; - this.targetValueType = targetValueType; read(in); } @@ -109,17 +107,14 @@ protected MultiValuesSourceAggregationBuilder(StreamInput in, ValueType targetVa @SuppressWarnings("unchecked") private void read(StreamInput in) throws IOException { fields = in.readMap(StreamInput::readString, MultiValuesSourceFieldConfig::new); - valueType = in.readOptionalWriteable(ValueType::readFromStream); + userValueTypeHint = in.readOptionalWriteable(ValueType::readFromStream); format = in.readOptionalString(); } @Override protected final void doWriteTo(StreamOutput out) throws IOException { - if (serializeTargetValueType()) { - out.writeOptionalWriteable(targetValueType); - } out.writeMap(fields, StreamOutput::writeString, (o, value) -> value.writeTo(o)); - out.writeOptionalWriteable(valueType); + out.writeOptionalWriteable(userValueTypeHint); out.writeOptionalString(format); innerWriteTo(out); } @@ -142,11 +137,11 @@ protected AB field(String propertyName, MultiValuesSourceFieldConfig config) { * Sets the {@link ValueType} for the value produced by this aggregation */ @SuppressWarnings("unchecked") - public AB valueType(ValueType valueType) { + public AB userValueTypeHint(ValueType valueType) { if (valueType == null) { - throw new IllegalArgumentException("[valueType] must not be null: [" + name + "]"); + throw new IllegalArgumentException("[userValueTypeHint] must not be null: [" + name + "]"); } - this.valueType = valueType; + this.userValueTypeHint = valueType; return (AB) this; } @@ -162,25 +157,34 @@ public AB format(String format) { return (AB) this; } + /** + * Aggregations should use this method to define a {@link ValuesSourceType} of last resort. This will only be used when the resolver + * can't find a field and the user hasn't provided a value type hint. + * + * @return The CoreValuesSourceType we expect this script to yield. + */ + protected abstract ValuesSourceType defaultValueSourceType(); + @Override protected final MultiValuesSourceAggregatorFactory doBuild(QueryShardContext queryShardContext, AggregatorFactory parent, Builder subFactoriesBuilder) throws IOException { - ValueType finalValueType = this.valueType != null ? this.valueType : targetValueType; - Map configs = new HashMap<>(fields.size()); fields.forEach((key, value) -> { - ValuesSourceConfig config = ValuesSourceConfig.resolve(queryShardContext, finalValueType, - value.getFieldName(), value.getScript(), value.getMissing(), value.getTimeZone(), format, getType()); + ValuesSourceConfig config = ValuesSourceConfig.resolve(queryShardContext, userValueTypeHint, + value.getFieldName(), value.getScript(), value.getMissing(), value.getTimeZone(), format, defaultValueSourceType(), + getType()); configs.put(key, config); }); - DocValueFormat docValueFormat = resolveFormat(format, finalValueType); + DocValueFormat docValueFormat = resolveFormat(format, userValueTypeHint, defaultValueSourceType()); return innerBuild(queryShardContext, configs, docValueFormat, parent, subFactoriesBuilder); } - private static DocValueFormat resolveFormat(@Nullable String format, @Nullable ValueType valueType) { + private static DocValueFormat resolveFormat(@Nullable String format, @Nullable ValueType valueType, + ValuesSourceType defaultValuesSourceType) { if (valueType == null) { - return DocValueFormat.RAW; // we can't figure it out + // If the user didn't send a hint, all we can do is fall back to the default + return defaultValuesSourceType.getFormatter(format, null); } DocValueFormat valueFormat = valueType.defaultFormat; if (valueFormat instanceof DocValueFormat.Decimal && format != null) { @@ -195,14 +199,6 @@ protected abstract MultiValuesSourceAggregatorFactory innerBuild(QueryShardConte Builder subFactoriesBuilder) 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. - */ - protected boolean serializeTargetValueType() { - return false; - } - @Override public final XContentBuilder internalXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); @@ -214,8 +210,8 @@ public final XContentBuilder internalXContent(XContentBuilder builder, Params pa if (format != null) { builder.field(CommonFields.FORMAT.getPreferredName(), format); } - if (valueType != null) { - builder.field(CommonFields.VALUE_TYPE.getPreferredName(), valueType.getPreferredName()); + if (userValueTypeHint != null) { + builder.field(CommonFields.VALUE_TYPE.getPreferredName(), userValueTypeHint.getPreferredName()); } doXContentBody(builder, params); builder.endObject(); @@ -226,7 +222,7 @@ public final XContentBuilder internalXContent(XContentBuilder builder, Params pa @Override public int hashCode() { - return Objects.hash(super.hashCode(), fields, format, targetValueType, valueType); + return Objects.hash(super.hashCode(), fields, format, userValueTypeHint); } @@ -239,6 +235,6 @@ public boolean equals(Object obj) { MultiValuesSourceAggregationBuilder other = (MultiValuesSourceAggregationBuilder) obj; return Objects.equals(this.fields, other.fields) && Objects.equals(this.format, other.format) - && Objects.equals(this.valueType, other.valueType); + && Objects.equals(this.userValueTypeHint, other.userValueTypeHint); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceParseHelper.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceParseHelper.java index 76f3327d77738..3bbb8bfb702fc 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceParseHelper.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceParseHelper.java @@ -31,7 +31,7 @@ public static void declareCommon( AbstractObjectParser, T> objectParser, boolean formattable, ValueType expectedValueType) { - objectParser.declareField(MultiValuesSourceAggregationBuilder::valueType, p -> { + objectParser.declareField(MultiValuesSourceAggregationBuilder::userValueTypeHint, p -> { ValueType valueType = ValueType.resolveForScript(p.text()); if (expectedValueType != null && valueType.isNotA(expectedValueType)) { throw new ParsingException(p.getTokenLocation(), 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 0605039c8baa2..43d9d158187eb 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 @@ -53,7 +53,7 @@ public enum ValueType implements Writeable { private final byte id; private String preferredName; - public static final ParseField VALUE_TYPE = new ParseField("value_type", "valueType"); + public static final ParseField VALUE_TYPE = new ParseField("value_type", "userValueTypeHint"); ValueType(byte id, String description, String preferredName, ValuesSourceType valuesSourceType, DocValueFormat defaultFormat) { @@ -114,7 +114,7 @@ public static ValueType readFromStream(StreamInput in) throws IOException { return valueType; } } - throw new IOException("No valueType found for id [" + id + "]"); + throw new IOException("No userValueTypeHint found for id [" + id + "]"); } @Override 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 e0a7917165a93..6e6213938c754 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 @@ -208,7 +208,7 @@ public AB userValueTypeHint(ValueType valueType) { // TODO: This is nonsense. We allow the value to be null (via constructor), but don't allow it to be set to null. This means // thing looking to copy settings (like RollupRequestTranslator) need to check if userValueTypeHint is not null, and then // set it if and only if it is non-null. - throw new IllegalArgumentException("[valueType] must not be null: [" + name + "]"); + throw new IllegalArgumentException("[userValueTypeHint] must not be null: [" + name + "]"); } this.userValueTypeHint = valueType; return (AB) this; @@ -286,8 +286,8 @@ protected final ValuesSourceAggregatorFactory doBuild(QueryShardContext querySha } /** - * Provide a hook for aggregations to have finer grained control of the {@link ValuesSourceType } when {@link ValuesSourceConfig} - * can't resolve a type (i.e. this will be called before falling back to CoreValuesSourceType.ANY.) + * Aggregations should use this method to define a {@link ValuesSourceType} of last resort. This will only be used when the resolver + * can't find a field and the user hasn't provided a value type hint. * * @return The CoreValuesSourceType we expect this script to yield. */ From 25f529b08d4aaedee4f587a0d040fb20b5760336 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 29 Jan 2020 15:15:47 -0500 Subject: [PATCH 4/5] Back out over-zealous intellij refactoring --- .../ArrayValuesSourceAggregationBuilder.java | 2 +- .../inject/multibindings/MapBinder.java | 12 +- .../common/inject/util/Types.java | 4 +- .../elasticsearch/search/SearchModule.java | 3 +- .../CompositeValuesSourceBuilder.java | 2 +- .../bucket/terms/TermsAggregationBuilder.java | 9 + .../bucket/terms/TermsAggregatorFactory.java | 239 ++++++++++++------ .../bucket/terms/TermsAggregatorSupplier.java | 51 ++++ .../terms/BinaryTermsAggregatorTests.java | 176 +++++++++++++ .../terms/KeywordTermsAggregatorTests.java | 161 ++++++++++++ .../terms/NumericTermsAggregatorTests.java | 190 ++++++++++++++ .../bucket/terms/TermsAggregatorTests.java | 6 +- 12 files changed, 757 insertions(+), 98 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorSupplier.java create mode 100644 server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/BinaryTermsAggregatorTests.java create mode 100644 server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/KeywordTermsAggregatorTests.java create mode 100644 server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/NumericTermsAggregatorTests.java diff --git a/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregationBuilder.java b/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregationBuilder.java index 3c4554e0ed112..3d86f71077eee 100644 --- a/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregationBuilder.java +++ b/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregationBuilder.java @@ -72,7 +72,7 @@ public AB subAggregations(Builder subFactories) { } private List fields = Collections.emptyList(); - /* The parser doesn't support setting userValueTypeHint (aka userValueTypeHint), but we do serialize and deserialize it, so keeping it around + /* The parser doesn't support setting userValueTypeHint (aka valueType), but we do serialize and deserialize it, so keeping it around for now so as to not break BWC. Future refactors should feel free to remove this field. --Tozzi 2020-01-16 */ private ValueType userValueTypeHint = null; diff --git a/server/src/main/java/org/elasticsearch/common/inject/multibindings/MapBinder.java b/server/src/main/java/org/elasticsearch/common/inject/multibindings/MapBinder.java index f7458c888a0e6..a0a22d96f58d5 100644 --- a/server/src/main/java/org/elasticsearch/common/inject/multibindings/MapBinder.java +++ b/server/src/main/java/org/elasticsearch/common/inject/multibindings/MapBinder.java @@ -99,7 +99,7 @@ private MapBinder() { } /** - * Returns a new mapbinder that collects entries of {@code keyType}/{@code userValueTypeHint} in a + * Returns a new mapbinder that collects entries of {@code keyType}/{@code valueType} in a * {@link Map} that is itself bound with no binding annotation. */ public static MapBinder newMapBinder(Binder binder, @@ -112,7 +112,7 @@ public static MapBinder newMapBinder(Binder binder, } /** - * Returns a new mapbinder that collects entries of {@code keyType}/{@code userValueTypeHint} in a + * Returns a new mapbinder that collects entries of {@code keyType}/{@code valueType} in a * {@link Map} that is itself bound with no binding annotation. */ public static MapBinder newMapBinder(Binder binder, @@ -121,7 +121,7 @@ public static MapBinder newMapBinder(Binder binder, } /** - * Returns a new mapbinder that collects entries of {@code keyType}/{@code userValueTypeHint} in a + * Returns a new mapbinder that collects entries of {@code keyType}/{@code valueType} in a * {@link Map} that is itself bound with {@code annotation}. */ public static MapBinder newMapBinder(Binder binder, @@ -134,7 +134,7 @@ public static MapBinder newMapBinder(Binder binder, } /** - * Returns a new mapbinder that collects entries of {@code keyType}/{@code userValueTypeHint} in a + * Returns a new mapbinder that collects entries of {@code keyType}/{@code valueType} in a * {@link Map} that is itself bound with {@code annotation}. */ public static MapBinder newMapBinder(Binder binder, @@ -143,7 +143,7 @@ public static MapBinder newMapBinder(Binder binder, } /** - * Returns a new mapbinder that collects entries of {@code keyType}/{@code userValueTypeHint} in a + * Returns a new mapbinder that collects entries of {@code keyType}/{@code valueType} in a * {@link Map} that is itself bound with {@code annotationType}. */ public static MapBinder newMapBinder(Binder binder, TypeLiteral keyType, @@ -156,7 +156,7 @@ public static MapBinder newMapBinder(Binder binder, TypeLiteral } /** - * Returns a new mapbinder that collects entries of {@code keyType}/{@code userValueTypeHint} in a + * Returns a new mapbinder that collects entries of {@code keyType}/{@code valueType} in a * {@link Map} that is itself bound with {@code annotationType}. */ public static MapBinder newMapBinder(Binder binder, Class keyType, diff --git a/server/src/main/java/org/elasticsearch/common/inject/util/Types.java b/server/src/main/java/org/elasticsearch/common/inject/util/Types.java index 05990502d948c..a3f42a0d374cc 100644 --- a/server/src/main/java/org/elasticsearch/common/inject/util/Types.java +++ b/server/src/main/java/org/elasticsearch/common/inject/util/Types.java @@ -113,7 +113,7 @@ public static ParameterizedType setOf(Type elementType) { /** * Returns a type modelling a {@link Map} whose keys are of type - * {@code keyType} and whose values are of type {@code userValueTypeHint}. + * {@code keyType} and whose values are of type {@code valueType}. * * @return a parameterized type. */ @@ -132,4 +132,4 @@ public static ParameterizedType mapOf(Type keyType, Type valueType) { public static ParameterizedType providerOf(Type providedType) { return newParameterizedType(Provider.class, providedType); } -} +} \ No newline at end of file diff --git a/server/src/main/java/org/elasticsearch/search/SearchModule.java b/server/src/main/java/org/elasticsearch/search/SearchModule.java index e170b0b8ad3d1..237a9e67162fa 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchModule.java +++ b/server/src/main/java/org/elasticsearch/search/SearchModule.java @@ -381,7 +381,8 @@ private void registerAggregations(List plugins) { .addResultReader(StringTerms.NAME, StringTerms::new) .addResultReader(UnmappedTerms.NAME, UnmappedTerms::new) .addResultReader(LongTerms.NAME, LongTerms::new) - .addResultReader(DoubleTerms.NAME, DoubleTerms::new)); + .addResultReader(DoubleTerms.NAME, DoubleTerms::new) + .setAggregatorRegistrar(TermsAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(RareTermsAggregationBuilder.NAME, RareTermsAggregationBuilder::new, RareTermsAggregationBuilder::parse) .addResultReader(StringRareTerms.NAME, StringRareTerms::new) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java index 04ec2c6d1460f..8507fad49bfd2 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java @@ -186,7 +186,7 @@ public Script script() { @SuppressWarnings("unchecked") public AB valueType(ValueType valueType) { if (valueType == null) { - throw new IllegalArgumentException("[userValueTypeHint] must not be null"); + throw new IllegalArgumentException("[valueType] must not be null"); } this.valueType = valueType; return (AB) this; 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 110929ffccb67..6a3b1da5f5020 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 @@ -42,12 +42,14 @@ import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper; +import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry; import org.elasticsearch.search.aggregations.support.ValuesSourceType; import java.io.IOException; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.concurrent.atomic.AtomicBoolean; public class TermsAggregationBuilder extends ValuesSourceAggregationBuilder implements MultiBucketAggregationBuilder { @@ -100,6 +102,13 @@ public static AggregationBuilder parse(String aggregationName, XContentParser pa return PARSER.parse(parser, new TermsAggregationBuilder(aggregationName), null); } + private static AtomicBoolean wasRegistered = new AtomicBoolean(false); + public static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { + if (wasRegistered.compareAndSet(false, true) == true) { + TermsAggregatorFactory.registerAggregators(valuesSourceRegistry); + } + } + private BucketOrder order = BucketOrder.compound(BucketOrder.count(false)); // automatically adds tie-breaker key asc order private IncludeExclude includeExclude = null; private String executionHint = null; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java index b39587aff480c..9880822b63c62 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java @@ -38,9 +38,12 @@ import org.elasticsearch.search.aggregations.bucket.BucketUtils; import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregator.BucketCountThresholds; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; +import org.elasticsearch.search.aggregations.support.AggregatorSupplier; +import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; import org.elasticsearch.search.aggregations.support.ValuesSource; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; +import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry; import org.elasticsearch.search.internal.SearchContext; import java.io.IOException; @@ -59,18 +62,141 @@ public class TermsAggregatorFactory extends ValuesSourceAggregatorFactory { private final TermsAggregator.BucketCountThresholds bucketCountThresholds; private final boolean showTermDocCountError; + // TODO: Registration should happen on the actual aggregator classes + static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { + valuesSourceRegistry.register(TermsAggregationBuilder.NAME, + List.of(CoreValuesSourceType.BYTES, CoreValuesSourceType.IP), + TermsAggregatorFactory.bytesSupplier()); + + valuesSourceRegistry.register(TermsAggregationBuilder.NAME, + List.of(CoreValuesSourceType.DATE, CoreValuesSourceType.BOOLEAN, CoreValuesSourceType.NUMERIC), + TermsAggregatorFactory.numericSupplier()); + } + + /** + * This supplier is used for all the field types that should be aggregated as bytes/strings, + * including those that need global ordinals + */ + private static TermsAggregatorSupplier bytesSupplier() { + return new TermsAggregatorSupplier() { + @Override + public Aggregator build(String name, + AggregatorFactories factories, + ValuesSource valuesSource, + BucketOrder order, + DocValueFormat format, + TermsAggregator.BucketCountThresholds bucketCountThresholds, + IncludeExclude includeExclude, + String executionHint, + SearchContext context, + Aggregator parent, + SubAggCollectionMode subAggCollectMode, + boolean showTermDocCountError, + List pipelineAggregators, + Map metaData) throws IOException { + + ExecutionMode execution = null; + if (executionHint != null) { + execution = ExecutionMode.fromString(executionHint, deprecationLogger); + } + // In some cases, using ordinals is just not supported: override it + if (valuesSource instanceof ValuesSource.Bytes.WithOrdinals == false) { + execution = ExecutionMode.MAP; + } + final long maxOrd = execution == ExecutionMode.GLOBAL_ORDINALS ? getMaxOrd(valuesSource, context.searcher()) : -1; + if (execution == null) { + execution = ExecutionMode.GLOBAL_ORDINALS; + } + if (subAggCollectMode == null) { + subAggCollectMode = SubAggCollectionMode.DEPTH_FIRST; + // TODO can we remove concept of AggregatorFactories.EMPTY? + if (factories != AggregatorFactories.EMPTY) { + subAggCollectMode = subAggCollectionMode(bucketCountThresholds.getShardSize(), maxOrd); + } + } + + if ((includeExclude != null) && (includeExclude.isRegexBased()) && format != DocValueFormat.RAW) { + // TODO this exception message is not really accurate for the string case. It's really disallowing regex + formatter + throw new AggregationExecutionException("Aggregation [" + name + "] cannot support regular expression style " + + "include/exclude settings as they can only be applied to string fields. Use an array of values for " + + "include/exclude clauses"); + } + + // TODO: [Zach] we might want refactor and remove ExecutionMode#create(), moving that logic outside the enum + return execution.create(name, factories, valuesSource, order, format, bucketCountThresholds, includeExclude, + context, parent, subAggCollectMode, showTermDocCountError, pipelineAggregators, metaData); + + } + }; + } + + /** + * This supplier is used for all fields that expect to be aggregated as a numeric value. + * This includes floating points, and formatted types that use numerics internally for storage (date, boolean, etc) + */ + private static TermsAggregatorSupplier numericSupplier() { + return new TermsAggregatorSupplier() { + @Override + public Aggregator build(String name, + AggregatorFactories factories, + ValuesSource valuesSource, + BucketOrder order, + DocValueFormat format, + TermsAggregator.BucketCountThresholds bucketCountThresholds, + IncludeExclude includeExclude, + String executionHint, + SearchContext context, + Aggregator parent, + SubAggCollectionMode subAggCollectMode, + boolean showTermDocCountError, + List pipelineAggregators, + Map metaData) throws IOException { + + if ((includeExclude != null) && (includeExclude.isRegexBased())) { + throw new AggregationExecutionException("Aggregation [" + name + "] cannot support regular expression style " + + "include/exclude settings as they can only be applied to string fields. Use an array of numeric values for " + + "include/exclude clauses used to filter numeric fields"); + } + + IncludeExclude.LongFilter longFilter = null; + if (subAggCollectMode == null) { + // TODO can we remove concept of AggregatorFactories.EMPTY? + if (factories != AggregatorFactories.EMPTY) { + subAggCollectMode = subAggCollectionMode(bucketCountThresholds.getShardSize(), -1); + } else { + subAggCollectMode = SubAggCollectionMode.DEPTH_FIRST; + } + } + if (((ValuesSource.Numeric) valuesSource).isFloatingPoint()) { + if (includeExclude != null) { + longFilter = includeExclude.convertToDoubleFilter(); + } + return new DoubleTermsAggregator(name, factories, (ValuesSource.Numeric) valuesSource, format, order, + bucketCountThresholds, context, parent, subAggCollectMode, showTermDocCountError, longFilter, + pipelineAggregators, metaData); + } + if (includeExclude != null) { + longFilter = includeExclude.convertToLongFilter(format); + } + return new LongTermsAggregator(name, factories, (ValuesSource.Numeric) valuesSource, format, order, + bucketCountThresholds, context, parent, subAggCollectMode, showTermDocCountError, longFilter, + pipelineAggregators, metaData); + } + }; + } + TermsAggregatorFactory(String name, - ValuesSourceConfig config, - BucketOrder order, - IncludeExclude includeExclude, - String executionHint, - SubAggCollectionMode collectMode, - TermsAggregator.BucketCountThresholds bucketCountThresholds, - boolean showTermDocCountError, - QueryShardContext queryShardContext, - AggregatorFactory parent, - AggregatorFactories.Builder subFactoriesBuilder, - Map metaData) throws IOException { + ValuesSourceConfig config, + BucketOrder order, + IncludeExclude includeExclude, + String executionHint, + SubAggCollectionMode collectMode, + TermsAggregator.BucketCountThresholds bucketCountThresholds, + boolean showTermDocCountError, + QueryShardContext queryShardContext, + AggregatorFactory parent, + AggregatorFactories.Builder subFactoriesBuilder, + Map metaData) throws IOException { super(name, config, queryShardContext, parent, subFactoriesBuilder, metaData); this.order = order; this.includeExclude = includeExclude; @@ -114,89 +240,36 @@ private static boolean isAggregationSort(BucketOrder order) { @Override protected Aggregator doCreateInternal(ValuesSource valuesSource, - SearchContext searchContext, - Aggregator parent, - boolean collectsFromSingleBucket, - List pipelineAggregators, - Map metaData) throws IOException { + SearchContext searchContext, + Aggregator parent, + boolean collectsFromSingleBucket, + List pipelineAggregators, + Map metaData) throws IOException { if (collectsFromSingleBucket == false) { return asMultiBucketAggregator(this, searchContext, parent); } + + AggregatorSupplier aggregatorSupplier = ValuesSourceRegistry.getInstance().getAggregator(config.valueSourceType(), + TermsAggregationBuilder.NAME); + if (aggregatorSupplier instanceof TermsAggregatorSupplier == false) { + throw new AggregationExecutionException("Registry miss-match - expected TermsAggregatorSupplier, found [" + + aggregatorSupplier.getClass().toString() + "]"); + } + + TermsAggregatorSupplier termsAggregatorSupplier = (TermsAggregatorSupplier) aggregatorSupplier; BucketCountThresholds bucketCountThresholds = new BucketCountThresholds(this.bucketCountThresholds); if (InternalOrder.isKeyOrder(order) == false - && bucketCountThresholds.getShardSize() == TermsAggregationBuilder.DEFAULT_BUCKET_COUNT_THRESHOLDS.getShardSize()) { + && bucketCountThresholds.getShardSize() == TermsAggregationBuilder.DEFAULT_BUCKET_COUNT_THRESHOLDS.getShardSize()) { // The user has not made a shardSize selection. Use default // heuristic to avoid any wrong-ranking caused by distributed // counting bucketCountThresholds.setShardSize(BucketUtils.suggestShardSideQueueSize(bucketCountThresholds.getRequiredSize())); } bucketCountThresholds.ensureValidity(); - if (valuesSource instanceof ValuesSource.Bytes) { - ExecutionMode execution = null; - if (executionHint != null) { - execution = ExecutionMode.fromString(executionHint, deprecationLogger); - } - // In some cases, using ordinals is just not supported: override it - if (valuesSource instanceof ValuesSource.Bytes.WithOrdinals == false) { - execution = ExecutionMode.MAP; - } - final long maxOrd = execution == ExecutionMode.GLOBAL_ORDINALS ? getMaxOrd(valuesSource, searchContext.searcher()) : -1; - if (execution == null) { - execution = ExecutionMode.GLOBAL_ORDINALS; - } - SubAggCollectionMode cm = collectMode; - if (cm == null) { - cm = SubAggCollectionMode.DEPTH_FIRST; - if (factories != AggregatorFactories.EMPTY) { - cm = subAggCollectionMode(bucketCountThresholds.getShardSize(), maxOrd); - } - } - - DocValueFormat format = config.format(); - if ((includeExclude != null) && (includeExclude.isRegexBased()) && format != DocValueFormat.RAW) { - throw new AggregationExecutionException("Aggregation [" + name + "] cannot support regular expression style " - + "include/exclude settings as they can only be applied to string fields. Use an array of values for " - + "include/exclude clauses"); - } - - return execution.create(name, factories, valuesSource, order, format, - bucketCountThresholds, includeExclude, searchContext, parent, cm, showTermDocCountError, pipelineAggregators, metaData); - } - - if ((includeExclude != null) && (includeExclude.isRegexBased())) { - throw new AggregationExecutionException("Aggregation [" + name + "] cannot support regular expression style " - + "include/exclude settings as they can only be applied to string fields. Use an array of numeric values for " - + "include/exclude clauses used to filter numeric fields"); - } - - if (valuesSource instanceof ValuesSource.Numeric) { - IncludeExclude.LongFilter longFilter = null; - SubAggCollectionMode cm = collectMode; - if (cm == null) { - if (factories != AggregatorFactories.EMPTY) { - cm = subAggCollectionMode(bucketCountThresholds.getShardSize(), -1); - } else { - cm = SubAggCollectionMode.DEPTH_FIRST; - } - } - if (((ValuesSource.Numeric) valuesSource).isFloatingPoint()) { - if (includeExclude != null) { - longFilter = includeExclude.convertToDoubleFilter(); - } - return new DoubleTermsAggregator(name, factories, (ValuesSource.Numeric) valuesSource, config.format(), order, - bucketCountThresholds, searchContext, parent, cm, showTermDocCountError, longFilter, - pipelineAggregators, metaData); - } - if (includeExclude != null) { - longFilter = includeExclude.convertToLongFilter(config.format()); - } - return new LongTermsAggregator(name, factories, (ValuesSource.Numeric) valuesSource, config.format(), order, - bucketCountThresholds, searchContext, parent, cm, showTermDocCountError, longFilter, pipelineAggregators, - metaData); - } - throw new AggregationExecutionException("terms aggregation cannot be applied to field [" + config.fieldContext().field() - + "]. It can only be applied to numeric or string fields."); + return termsAggregatorSupplier.build(name, factories, valuesSource, order, config.format(), + bucketCountThresholds, includeExclude, executionHint, searchContext, parent, collectMode, + showTermDocCountError, pipelineAggregators, metaData); } // return the SubAggCollectionMode that this aggregation should use based on the expected size @@ -217,7 +290,7 @@ static SubAggCollectionMode subAggCollectionMode(int expectedSize, long maxOrd) * Get the maximum global ordinal value for the provided {@link ValuesSource} or -1 * if the values source is not an instance of {@link ValuesSource.Bytes.WithOrdinals}. */ - static long getMaxOrd(ValuesSource source, IndexSearcher searcher) throws IOException { + private static long getMaxOrd(ValuesSource source, IndexSearcher searcher) throws IOException { if (source instanceof ValuesSource.Bytes.WithOrdinals) { ValuesSource.Bytes.WithOrdinals valueSourceWithOrdinals = (ValuesSource.Bytes.WithOrdinals) source; return valueSourceWithOrdinals.globalMaxOrd(searcher); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorSupplier.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorSupplier.java new file mode 100644 index 0000000000000..ccfb082ac98ce --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorSupplier.java @@ -0,0 +1,51 @@ +/* + * 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.terms; + +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.bucket.terms.IncludeExclude; +import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregator; +import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; +import org.elasticsearch.search.aggregations.support.AggregatorSupplier; +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; + +interface TermsAggregatorSupplier extends AggregatorSupplier { + Aggregator build(String name, + AggregatorFactories factories, + ValuesSource valuesSource, + BucketOrder order, + DocValueFormat format, + TermsAggregator.BucketCountThresholds bucketCountThresholds, + IncludeExclude includeExclude, + String executionHint, + SearchContext context, + Aggregator parent, + Aggregator.SubAggCollectionMode subAggCollectMode, + boolean showTermDocCountError, + List pipelineAggregators, + Map metaData) throws IOException; +} diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/BinaryTermsAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/BinaryTermsAggregatorTests.java new file mode 100644 index 0000000000000..31859a7a4fa04 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/BinaryTermsAggregatorTests.java @@ -0,0 +1,176 @@ +/* + * 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.terms; + +import org.apache.lucene.document.Document; +import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.index.IndexReader; +import org.apache.lucene.index.RandomIndexWriter; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.MatchAllDocsQuery; +import org.apache.lucene.search.MatchNoDocsQuery; +import org.apache.lucene.search.Query; +import org.apache.lucene.store.Directory; +import org.apache.lucene.util.BytesRef; +import org.apache.lucene.util.automaton.RegExp; +import org.elasticsearch.common.Numbers; +import org.elasticsearch.index.mapper.BinaryFieldMapper; +import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.search.DocValueFormat; +import org.elasticsearch.search.aggregations.AggregationExecutionException; +import org.elasticsearch.search.aggregations.AggregatorTestCase; +import org.elasticsearch.search.aggregations.support.ValueType; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.function.Consumer; + +import static org.hamcrest.Matchers.equalTo; + +public class BinaryTermsAggregatorTests extends AggregatorTestCase { + private static final String BINARY_FIELD = "binary"; + + private static final List dataset; + static { + List d = new ArrayList<>(45); + for (int i = 0; i < 10; i++) { + for (int j = 0; j < i; j++) { + d.add((long) i); + } + } + dataset = d; + } + + public void testMatchNoDocs() throws IOException { + testBothCases(new MatchNoDocsQuery(), dataset, + aggregation -> aggregation.field(BINARY_FIELD), + agg -> assertEquals(0, agg.getBuckets().size()), ValueType.STRING + ); + } + + public void testMatchAllDocs() throws IOException { + Query query = new MatchAllDocsQuery(); + + testBothCases(query, dataset, + aggregation -> aggregation.field(BINARY_FIELD), + agg -> { + assertEquals(9, agg.getBuckets().size()); + for (int i = 0; i < 9; i++) { + StringTerms.Bucket bucket = (StringTerms.Bucket) agg.getBuckets().get(i); + byte[] bytes = Numbers.longToBytes(9L - i); + String bytesAsString = (String) DocValueFormat.BINARY.format(new BytesRef(bytes)); + assertThat(bucket.getKey(), equalTo(bytesAsString)); + assertThat(bucket.getDocCount(), equalTo(9L - i)); + } + }, null); + } + + public void testBadIncludeExclude() throws IOException { + IncludeExclude includeExclude = new IncludeExclude(new RegExp("foo"), null); + + // Make sure the include/exclude fails regardless of how the user tries to type hint the agg + AggregationExecutionException e = expectThrows(AggregationExecutionException.class, + () -> testBothCases(new MatchNoDocsQuery(), dataset, + aggregation -> aggregation.field(BINARY_FIELD).includeExclude(includeExclude).format("yyyy-MM-dd"), + agg -> fail("test should have failed with exception"), null // default, no hint + )); + assertThat(e.getMessage(), equalTo("Aggregation [_name] cannot support regular expression style include/exclude settings as " + + "they can only be applied to string fields. Use an array of values for include/exclude clauses")); + + e = expectThrows(AggregationExecutionException.class, + () -> testBothCases(new MatchNoDocsQuery(), dataset, + aggregation -> aggregation.field(BINARY_FIELD).includeExclude(includeExclude).format("yyyy-MM-dd"), + agg -> fail("test should have failed with exception"), ValueType.STRING // string type hint + )); + assertThat(e.getMessage(), equalTo("Aggregation [_name] cannot support regular expression style include/exclude settings as " + + "they can only be applied to string fields. Use an array of values for include/exclude clauses")); + + e = expectThrows(AggregationExecutionException.class, () -> testBothCases(new MatchNoDocsQuery(), dataset, + aggregation -> aggregation.field(BINARY_FIELD).includeExclude(includeExclude), + agg -> fail("test should have failed with exception"), ValueType.NUMERIC // numeric type hint + )); + assertThat(e.getMessage(), equalTo("Aggregation [_name] cannot support regular expression style include/exclude settings as " + + "they can only be applied to string fields. Use an array of values for include/exclude clauses")); + } + + private void testSearchCase(Query query, List dataset, + Consumer configure, + Consumer verify, ValueType valueType) throws IOException { + executeTestCase(false, query, dataset, configure, verify, valueType); + } + + private void testSearchAndReduceCase(Query query, List dataset, + Consumer configure, + Consumer verify, ValueType valueType) throws IOException { + executeTestCase(true, query, dataset, configure, verify, valueType); + } + + private void testBothCases(Query query, List dataset, + Consumer configure, + Consumer verify, ValueType valueType) throws IOException { + testSearchCase(query, dataset, configure, verify, valueType); + testSearchAndReduceCase(query, dataset, configure, verify, valueType); + } + + private void executeTestCase(boolean reduced, Query query, List dataset, + Consumer configure, + Consumer verify, ValueType valueType) throws IOException { + + try (Directory directory = newDirectory()) { + try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) { + Document document = new Document(); + for (Long value : dataset) { + if (frequently()) { + indexWriter.commit(); + } + + document.add(new BinaryFieldMapper.CustomBinaryDocValuesField(BINARY_FIELD, Numbers.longToBytes(value))); + indexWriter.addDocument(document); + document.clear(); + } + } + + try (IndexReader indexReader = DirectoryReader.open(directory)) { + IndexSearcher indexSearcher = newIndexSearcher(indexReader); + + TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("_name"); + if (valueType != null) { + aggregationBuilder.userValueTypeHint(valueType); + } + if (configure != null) { + configure.accept(aggregationBuilder); + } + + MappedFieldType binaryFieldType = new BinaryFieldMapper.Builder(BINARY_FIELD).fieldType(); + binaryFieldType.setName(BINARY_FIELD); + binaryFieldType.setHasDocValues(true); + + InternalMappedTerms rareTerms; + if (reduced) { + rareTerms = searchAndReduce(indexSearcher, query, aggregationBuilder, binaryFieldType); + } else { + rareTerms = search(indexSearcher, query, aggregationBuilder, binaryFieldType); + } + verify.accept(rareTerms); + } + } + } + +} diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/KeywordTermsAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/KeywordTermsAggregatorTests.java new file mode 100644 index 0000000000000..098e94d3d4450 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/KeywordTermsAggregatorTests.java @@ -0,0 +1,161 @@ +/* + * 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.terms; + +import org.apache.lucene.document.Document; +import org.apache.lucene.document.SortedSetDocValuesField; +import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.index.IndexReader; +import org.apache.lucene.index.RandomIndexWriter; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.MatchAllDocsQuery; +import org.apache.lucene.search.MatchNoDocsQuery; +import org.apache.lucene.search.Query; +import org.apache.lucene.store.Directory; +import org.apache.lucene.util.BytesRef; +import org.elasticsearch.index.mapper.KeywordFieldMapper; +import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.search.aggregations.AggregatorTestCase; +import org.elasticsearch.search.aggregations.support.ValueType; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.function.Consumer; + +import static org.hamcrest.Matchers.equalTo; + +public class KeywordTermsAggregatorTests extends AggregatorTestCase { + private static final String KEYWORD_FIELD = "keyword"; + + private static final List dataset; + static { + List d = new ArrayList<>(45); + for (int i = 0; i < 10; i++) { + for (int j = 0; j < i; j++) { + d.add(String.valueOf(i)); + } + } + dataset = d; + } + + public void testMatchNoDocs() throws IOException { + testBothCases(new MatchNoDocsQuery(), dataset, + aggregation -> aggregation.field(KEYWORD_FIELD), + agg -> assertEquals(0, agg.getBuckets().size()), null // without type hint + ); + + testBothCases(new MatchNoDocsQuery(), dataset, + aggregation -> aggregation.field(KEYWORD_FIELD), + agg -> assertEquals(0, agg.getBuckets().size()), ValueType.STRING // with type hint + ); + } + + public void testMatchAllDocs() throws IOException { + Query query = new MatchAllDocsQuery(); + + testBothCases(query, dataset, + aggregation -> aggregation.field(KEYWORD_FIELD), + agg -> { + assertEquals(9, agg.getBuckets().size()); + for (int i = 0; i < 9; i++) { + StringTerms.Bucket bucket = (StringTerms.Bucket) agg.getBuckets().get(i); + assertThat(bucket.getKey(), equalTo(String.valueOf(9L - i))); + assertThat(bucket.getDocCount(), equalTo(9L - i)); + } + }, null // without type hint + ); + + testBothCases(query, dataset, + aggregation -> aggregation.field(KEYWORD_FIELD), + agg -> { + assertEquals(9, agg.getBuckets().size()); + for (int i = 0; i < 9; i++) { + StringTerms.Bucket bucket = (StringTerms.Bucket) agg.getBuckets().get(i); + assertThat(bucket.getKey(), equalTo(String.valueOf(9L - i))); + assertThat(bucket.getDocCount(), equalTo(9L - i)); + } + }, ValueType.STRING // with type hint + ); + } + + private void testSearchCase(Query query, List dataset, + Consumer configure, + Consumer verify, ValueType valueType) throws IOException { + executeTestCase(false, query, dataset, configure, verify, valueType); + } + + private void testSearchAndReduceCase(Query query, List dataset, + Consumer configure, + Consumer verify, ValueType valueType) throws IOException { + executeTestCase(true, query, dataset, configure, verify, valueType); + } + + private void testBothCases(Query query, List dataset, + Consumer configure, + Consumer verify, ValueType valueType) throws IOException { + testSearchCase(query, dataset, configure, verify, valueType); + testSearchAndReduceCase(query, dataset, configure, verify, valueType); + } + + private void executeTestCase(boolean reduced, Query query, List dataset, + Consumer configure, + Consumer verify, ValueType valueType) throws IOException { + + try (Directory directory = newDirectory()) { + try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) { + Document document = new Document(); + for (String value : dataset) { + if (frequently()) { + indexWriter.commit(); + } + + document.add(new SortedSetDocValuesField(KEYWORD_FIELD, new BytesRef(value))); + indexWriter.addDocument(document); + document.clear(); + } + } + + try (IndexReader indexReader = DirectoryReader.open(directory)) { + IndexSearcher indexSearcher = newIndexSearcher(indexReader); + + TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("_name"); + if (valueType != null) { + aggregationBuilder.userValueTypeHint(valueType); + } + if (configure != null) { + configure.accept(aggregationBuilder); + } + + MappedFieldType keywordFieldType = new KeywordFieldMapper.KeywordFieldType(); + keywordFieldType.setName(KEYWORD_FIELD); + keywordFieldType.setHasDocValues(true); + + InternalMappedTerms rareTerms; + if (reduced) { + rareTerms = searchAndReduce(indexSearcher, query, aggregationBuilder, keywordFieldType); + } else { + rareTerms = search(indexSearcher, query, aggregationBuilder, keywordFieldType); + } + verify.accept(rareTerms); + } + } + } + +} diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/NumericTermsAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/NumericTermsAggregatorTests.java new file mode 100644 index 0000000000000..b0790b9fa0413 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/NumericTermsAggregatorTests.java @@ -0,0 +1,190 @@ +/* + * 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.terms; + +import org.apache.lucene.document.Document; +import org.apache.lucene.document.LongPoint; +import org.apache.lucene.document.SortedNumericDocValuesField; +import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.index.IndexReader; +import org.apache.lucene.index.RandomIndexWriter; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.MatchAllDocsQuery; +import org.apache.lucene.search.MatchNoDocsQuery; +import org.apache.lucene.search.Query; +import org.apache.lucene.store.Directory; +import org.apache.lucene.util.automaton.RegExp; +import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.index.mapper.NumberFieldMapper; +import org.elasticsearch.search.aggregations.AggregationExecutionException; +import org.elasticsearch.search.aggregations.AggregatorTestCase; +import org.elasticsearch.search.aggregations.support.ValueType; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.function.Consumer; + +import static org.hamcrest.Matchers.equalTo; + +public class NumericTermsAggregatorTests extends AggregatorTestCase { + private static final String LONG_FIELD = "long"; + + private static final List dataset; + static { + List d = new ArrayList<>(45); + for (int i = 0; i < 10; i++) { + for (int j = 0; j < i; j++) { + d.add((long) i); + } + } + dataset = d; + } + + public void testMatchNoDocs() throws IOException { + + testBothCases(new MatchNoDocsQuery(), dataset, + aggregation -> aggregation.field(LONG_FIELD), + agg -> assertEquals(0, agg.getBuckets().size()), null // without type hint + ); + + testBothCases(new MatchNoDocsQuery(), dataset, + aggregation -> aggregation.field(LONG_FIELD), + agg -> assertEquals(0, agg.getBuckets().size()), ValueType.NUMERIC // with type hint + ); + } + + public void testMatchAllDocs() throws IOException { + Query query = new MatchAllDocsQuery(); + + testBothCases(query, dataset, + aggregation -> aggregation.field(LONG_FIELD), + agg -> { + assertEquals(9, agg.getBuckets().size()); + for (int i = 0; i < 9; i++) { + LongTerms.Bucket bucket = (LongTerms.Bucket) agg.getBuckets().get(i); + assertThat(bucket.getKey(), equalTo(9L - i)); + assertThat(bucket.getDocCount(), equalTo(9L - i)); + } + }, null //without type hint + ); + + testBothCases(query, dataset, + aggregation -> aggregation.field(LONG_FIELD), + agg -> { + assertEquals(9, agg.getBuckets().size()); + for (int i = 0; i < 9; i++) { + LongTerms.Bucket bucket = (LongTerms.Bucket) agg.getBuckets().get(i); + assertThat(bucket.getKey(), equalTo(9L - i)); + assertThat(bucket.getDocCount(), equalTo(9L - i)); + } + }, ValueType.NUMERIC //with type hint + ); + } + + public void testBadIncludeExclude() throws IOException { + IncludeExclude includeExclude = new IncludeExclude(new RegExp("foo"), null); + + // Numerics don't support any regex include/exclude, so should fail no matter what we do + + AggregationExecutionException e = expectThrows(AggregationExecutionException.class, + () -> testBothCases(new MatchNoDocsQuery(), dataset, + aggregation -> aggregation.field(LONG_FIELD).includeExclude(includeExclude).format("yyyy-MM-dd"), + agg -> fail("test should have failed with exception"), null + )); + assertThat(e.getMessage(), equalTo("Aggregation [_name] cannot support regular expression style " + + "include/exclude settings as they can only be applied to string fields. Use an array of numeric " + + "values for include/exclude clauses used to filter numeric fields")); + + e = expectThrows(AggregationExecutionException.class, + () -> testBothCases(new MatchNoDocsQuery(), dataset, + aggregation -> aggregation.field(LONG_FIELD).includeExclude(includeExclude).format("yyyy-MM-dd"), + agg -> fail("test should have failed with exception"), ValueType.NUMERIC // with type hint + )); + assertThat(e.getMessage(), equalTo("Aggregation [_name] cannot support regular expression style " + + "include/exclude settings as they can only be applied to string fields. Use an array of numeric " + + "values for include/exclude clauses used to filter numeric fields")); + + } + + private void testSearchCase(Query query, List dataset, + Consumer configure, + Consumer verify, ValueType valueType) throws IOException { + executeTestCase(false, query, dataset, configure, verify, valueType); + } + + private void testSearchAndReduceCase(Query query, List dataset, + Consumer configure, + Consumer verify, ValueType valueType) throws IOException { + executeTestCase(true, query, dataset, configure, verify, valueType); + } + + private void testBothCases(Query query, List dataset, + Consumer configure, + Consumer verify, ValueType valueType) throws IOException { + testSearchCase(query, dataset, configure, verify, valueType); + testSearchAndReduceCase(query, dataset, configure, verify, valueType); + } + + private void executeTestCase(boolean reduced, Query query, List dataset, + Consumer configure, + Consumer verify, ValueType valueType) throws IOException { + + try (Directory directory = newDirectory()) { + try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) { + Document document = new Document(); + for (Long value : dataset) { + if (frequently()) { + indexWriter.commit(); + } + + document.add(new SortedNumericDocValuesField(LONG_FIELD, value)); + document.add(new LongPoint(LONG_FIELD, value)); + indexWriter.addDocument(document); + document.clear(); + } + } + + try (IndexReader indexReader = DirectoryReader.open(directory)) { + IndexSearcher indexSearcher = newIndexSearcher(indexReader); + + TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("_name"); + if (valueType != null) { + aggregationBuilder.userValueTypeHint(valueType); + } + if (configure != null) { + configure.accept(aggregationBuilder); + } + + MappedFieldType longFieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG); + longFieldType.setName(LONG_FIELD); + longFieldType.setHasDocValues(true); + + InternalMappedTerms rareTerms; + if (reduced) { + rareTerms = searchAndReduce(indexSearcher, query, aggregationBuilder, longFieldType); + } else { + rareTerms = search(indexSearcher, query, aggregationBuilder, longFieldType); + } + verify.accept(rareTerms); + } + } + } + +} diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java index b4aedb3061f0b..b2ce863a9df57 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java @@ -920,8 +920,7 @@ public void testRangeField() throws Exception { IndexSearcher indexSearcher = newIndexSearcher(indexReader); TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("_name") .field(fieldName); - // Note - other places we throw IllegalArgumentException - expectThrows(AggregationExecutionException.class, () -> { + expectThrows(IllegalArgumentException.class, () -> { createAggregator(aggregationBuilder, indexSearcher, fieldType); }); } @@ -944,8 +943,7 @@ public void testGeoPointField() throws Exception { IndexSearcher indexSearcher = newIndexSearcher(indexReader); TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("_name") .field(field); - // Note - other places we throw IllegalArgumentException - expectThrows(AggregationExecutionException.class, () -> { + expectThrows(IllegalArgumentException.class, () -> { createAggregator(aggregationBuilder, indexSearcher, fieldType); }); } From 5d44f8ac4cf51baee6a9118679d32e5b8f638f20 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 29 Jan 2020 15:22:58 -0500 Subject: [PATCH 5/5] Back out over-zealous intellij refactoring, part two --- .../elasticsearch/search/aggregations/support/ValueType.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 43d9d158187eb..c2fbab66028ce 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 @@ -53,7 +53,7 @@ public enum ValueType implements Writeable { private final byte id; private String preferredName; - public static final ParseField VALUE_TYPE = new ParseField("value_type", "userValueTypeHint"); + public static final ParseField VALUE_TYPE = new ParseField("value_type", "valueType"); ValueType(byte id, String description, String preferredName, ValuesSourceType valuesSourceType, DocValueFormat defaultFormat) { @@ -114,7 +114,7 @@ public static ValueType readFromStream(StreamInput in) throws IOException { return valueType; } } - throw new IOException("No userValueTypeHint found for id [" + id + "]"); + throw new IOException("No ValueType found for id [" + id + "]"); } @Override