Skip to content

Value type hint as string #80838

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -180,10 +180,10 @@ public void testAggregationUsage() throws IOException {

Map<String, Map<String, Long>> afterCombinedAggsUsage = getTotalUsage(nodesMap);

assertDiff(beforeCombinedAggsUsage, afterCombinedAggsUsage, "terms", "numeric", 1L);
assertDiff(beforeCombinedAggsUsage, afterCombinedAggsUsage, "terms", "long", 1L);
assertDiff(beforeCombinedAggsUsage, afterCombinedAggsUsage, "terms", "date", 1L);
assertDiff(beforeCombinedAggsUsage, afterCombinedAggsUsage, "terms", "keyword", 2L);
assertDiff(beforeCombinedAggsUsage, afterCombinedAggsUsage, "avg", "numeric", 3L);
assertDiff(beforeCombinedAggsUsage, afterCombinedAggsUsage, "avg", "long", 3L);
Copy link
Member

Choose a reason for hiding this comment

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

Does this one also want to have double here?

}

private void assertDiff(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ protected Map<String, ValuesSourceConfig> resolveConfig(AggregationContext conte
for (String field : fields) {
ValuesSourceConfig config = ValuesSourceConfig.resolveUnregistered(
context,
userValueTypeHint,
userValueTypeHint == null ? null : userValueTypeHint.getValuesSourceType(),
field,
null,
missingMap.get(field),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@ public abstract class ArrayValuesSourceParser<VS extends ValuesSource> implement

public abstract static class NumericValuesSourceParser extends ArrayValuesSourceParser<ValuesSource.Numeric> {

// TODO: Long Support? IDK what we should do here.
protected NumericValuesSourceParser(boolean formattable) {
super(formattable, CoreValuesSourceType.NUMERIC, ValueType.NUMERIC);
super(formattable, CoreValuesSourceType.DOUBLE, ValueType.NUMERIC);
Copy link
Member

Choose a reason for hiding this comment

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

It's probably best not to drop support for anything in a big refactoring PR. Maybe make sure we have a test that targets a long field?

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import org.elasticsearch.search.aggregations.support.ValueType;
import org.elasticsearch.search.builder.SearchSourceBuilder;
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.xcontent.XContentParseException;
import org.elasticsearch.xcontent.XContentParser;
import org.elasticsearch.xcontent.json.JsonXContent;
import org.junit.After;
Expand Down Expand Up @@ -1389,16 +1388,20 @@ public void testScriptWithValueType() throws Exception {
assertThat(terms.getBuckets().size(), equalTo(1));
}

String invalidValueType = source.replaceAll("\"value_type\":\"n.*\"", "\"value_type\":\"foobar\"");
String invalidValueType = source.replaceAll("\"value_type\":\"n.*?\"", "\"value_type\":\"foobar\"");
Copy link
Member Author

Choose a reason for hiding this comment

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

Regexes are hard. This test has always produced invalid json here, but apparently in the old code path we failed on value type validation first. Stream parsing I guess.


try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidValueType)) {
XContentParseException ex = expectThrows(
XContentParseException.class,
SearchPhaseExecutionException ex = expectThrows(
SearchPhaseExecutionException.class,
Copy link
Member Author

Choose a reason for hiding this comment

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

We no longer catch invalid value types at parse time. We do still catch them before aggregation execution though (at ValuesSourceConfig#Resolve time).

() -> client().prepareSearch("idx").setSource(SearchSourceBuilder.fromXContent(parser)).get()
);
assertThat(ex.getCause(), instanceOf(IllegalArgumentException.class));

assertThat(ex.getCause(), instanceOf(ElasticsearchException.class));
assertThat(ex.getCause().getMessage(), containsString("Unknown value type [foobar]"));

assertThat(ex.getCause().getCause(), instanceOf(IllegalArgumentException.class));
assertThat(ex.getCause().getCause().getMessage(), containsString("Unknown value type [foobar]"));

}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public String getFieldName() {

@Override
public ValuesSourceType getValuesSourceType() {
return CoreValuesSourceType.NUMERIC;
return CoreValuesSourceType.DOUBLE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,15 @@ public abstract class IndexNumericFieldData implements IndexFieldData<LeafNumeri
*/
public enum NumericType {
BOOLEAN(false, SortField.Type.LONG, CoreValuesSourceType.BOOLEAN),
BYTE(false, SortField.Type.LONG, CoreValuesSourceType.NUMERIC),
SHORT(false, SortField.Type.LONG, CoreValuesSourceType.NUMERIC),
INT(false, SortField.Type.LONG, CoreValuesSourceType.NUMERIC),
LONG(false, SortField.Type.LONG, CoreValuesSourceType.NUMERIC),
BYTE(false, SortField.Type.LONG, CoreValuesSourceType.LONG),
SHORT(false, SortField.Type.LONG, CoreValuesSourceType.LONG),
INT(false, SortField.Type.LONG, CoreValuesSourceType.LONG),
LONG(false, SortField.Type.LONG, CoreValuesSourceType.LONG),
DATE(false, SortField.Type.LONG, CoreValuesSourceType.DATE),
DATE_NANOSECONDS(false, SortField.Type.LONG, CoreValuesSourceType.DATE),
HALF_FLOAT(true, SortField.Type.LONG, CoreValuesSourceType.NUMERIC),
FLOAT(true, SortField.Type.FLOAT, CoreValuesSourceType.NUMERIC),
DOUBLE(true, SortField.Type.DOUBLE, CoreValuesSourceType.NUMERIC);
HALF_FLOAT(true, SortField.Type.LONG, CoreValuesSourceType.DOUBLE),
FLOAT(true, SortField.Type.FLOAT, CoreValuesSourceType.DOUBLE),
DOUBLE(true, SortField.Type.DOUBLE, CoreValuesSourceType.DOUBLE);

private final boolean floatingPoint;
private final ValuesSourceType valuesSourceType;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public String getFieldName() {

@Override
public ValuesSourceType getValuesSourceType() {
return CoreValuesSourceType.NUMERIC;
return CoreValuesSourceType.DOUBLE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.elasticsearch.search.aggregations.bucket.terms.heuristic.SignificanceHeuristic;
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;
import org.elasticsearch.search.fetch.FetchSubPhase;
import org.elasticsearch.search.fetch.subphase.highlight.Highlighter;
import org.elasticsearch.search.internal.ShardSearchRequest;
Expand Down Expand Up @@ -115,6 +116,13 @@ default List<AggregationSpec> getAggregations() {
return emptyList();
}

/**
* The new {@link ValuesSourceType}s added by this plugin
*/
default List<ValuesSourceType> getValuesSourceTypes() {
return emptyList();
}

/**
* Allows plugins to register new aggregations using aggregation names that are already defined
* in Core, as long as the new aggregations target different ValuesSourceTypes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,9 @@
import org.elasticsearch.search.aggregations.pipeline.SerialDiffPipelineAggregationBuilder;
import org.elasticsearch.search.aggregations.pipeline.StatsBucketPipelineAggregationBuilder;
import org.elasticsearch.search.aggregations.pipeline.SumBucketPipelineAggregationBuilder;
import org.elasticsearch.search.aggregations.support.CoreValuesSourceType;
import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry;
import org.elasticsearch.search.aggregations.support.ValuesSourceType;
import org.elasticsearch.search.fetch.FetchPhase;
import org.elasticsearch.search.fetch.FetchSubPhase;
import org.elasticsearch.search.fetch.subphase.ExplainPhase;
Expand Down Expand Up @@ -343,6 +345,10 @@ public Map<String, Highlighter> getHighlighters() {
private ValuesSourceRegistry registerAggregations(List<SearchPlugin> plugins) {
ValuesSourceRegistry.Builder builder = new ValuesSourceRegistry.Builder();

for (ValuesSourceType coreVST : CoreValuesSourceType.values()) {
builder.registerValuesSourceType(coreVST);
}

registerAggregation(
new AggregationSpec(AvgAggregationBuilder.NAME, AvgAggregationBuilder::new, AvgAggregationBuilder.PARSER).addResultReader(
InternalAvg::new
Expand Down Expand Up @@ -645,6 +651,9 @@ private ValuesSourceRegistry registerAggregations(List<SearchPlugin> plugins) {
);
}

// Before registering plugin aggregations, register plugin values sources
registerFromPlugin(plugins, SearchPlugin::getValuesSourceTypes, builder::registerValuesSourceType);

registerFromPlugin(plugins, SearchPlugin::getAggregations, (agg) -> this.registerAggregation(agg, builder));

// after aggs have been registered, see if there are any new VSTypes that need to be linked to core fields
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public abstract class CompositeValuesSourceBuilder<AB extends CompositeValuesSou
protected final String name;
private String field = null;
private Script script = null;
private ValueType userValueTypeHint = null;
private String userValueTypeHint = null;
private boolean missingBucket = false;
private MissingOrder missingOrder = MissingOrder.DEFAULT;
private SortOrder order = SortOrder.ASC;
Expand All @@ -52,7 +52,12 @@ public abstract class CompositeValuesSourceBuilder<AB extends CompositeValuesSou
this.script = new Script(in);
}
if (in.readBoolean()) {
this.userValueTypeHint = ValueType.readFromStream(in);
if (in.getVersion().before(Version.V_8_1_0)) {
// legacy version wrote a ValueType. Convert it to a string
userValueTypeHint = ValueType.readFromStream(in).getPreferredName();
} else {
userValueTypeHint = in.readString();
}
}
this.missingBucket = in.readBoolean();
if (in.getVersion().onOrAfter(Version.V_7_16_0)) {
Expand All @@ -74,7 +79,17 @@ public final void writeTo(StreamOutput out) throws IOException {
boolean hasValueType = userValueTypeHint != null;
out.writeBoolean(hasValueType);
if (hasValueType) {
userValueTypeHint.writeTo(out);
if (out.getVersion().before(Version.V_8_1_0)) {
// Legacy version writes a ValueType
ValueType toWrite = ValueType.lenientParse(userValueTypeHint);
if (toWrite != null) {
toWrite.writeTo(out);
} else {
throw new IOException("Value_type [" + userValueTypeHint + "] not compatible with pre 8.1 nodes");
}
} else {
out.writeString(userValueTypeHint);
}
}
out.writeBoolean(missingBucket);
if (out.getVersion().onOrAfter(Version.V_7_16_0)) {
Expand Down Expand Up @@ -103,7 +118,7 @@ public final XContentBuilder toXContent(XContentBuilder builder, Params params)
builder.field("missing_order", missingOrder.toString());
}
if (userValueTypeHint != null) {
builder.field("value_type", userValueTypeHint.getPreferredName());
builder.field("value_type", userValueTypeHint);
}
if (format != null) {
builder.field("format", format);
Expand Down Expand Up @@ -183,18 +198,31 @@ public Script script() {
* Sets the {@link ValueType} for the value produced by this source
*/
@SuppressWarnings("unchecked")
public AB userValuetypeHint(ValueType valueType) {
public AB userValuetypeHint(String valueType) {
if (valueType == null) {
throw new IllegalArgumentException("[userValueTypeHint] must not be null");
}
this.userValueTypeHint = valueType;
return (AB) this;
}

/**
* Sets the {@link ValueType} for the value produced by this source
*/
@SuppressWarnings("unchecked")
@Deprecated
public AB userValuetypeHint(ValueType valueType) {
if (valueType == null) {
throw new IllegalArgumentException("[userValueTypeHint] must not be null");
}
this.userValueTypeHint = valueType.getPreferredName();
return (AB) this;
}

/**
* Gets the {@link ValueType} for the value produced by this source
*/
public ValueType userValuetypeHint() {
public String userValuetypeHint() {
return userValueTypeHint;
}

Expand Down Expand Up @@ -307,7 +335,7 @@ public final CompositeValuesSourceConfig build(AggregationContext context) throw

ValuesSourceConfig config = ValuesSourceConfig.resolve(
context,
userValueTypeHint,
context.getValuesSourceRegistry().resolveTypeHint(userValueTypeHint),
field,
script,
null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.script.Script;
import org.elasticsearch.search.aggregations.support.ValueType;
import org.elasticsearch.xcontent.AbstractObjectParser;
import org.elasticsearch.xcontent.ObjectParser;
import org.elasticsearch.xcontent.ParseField;
Expand All @@ -32,10 +31,7 @@ static <VB extends CompositeValuesSourceBuilder<VB>, T> void declareValuesSource
objectParser.declareBoolean(VB::missingBucket, new ParseField("missing_bucket"));
objectParser.declareString(VB::missingOrder, new ParseField("missing_order"));

objectParser.declareField(VB::userValuetypeHint, p -> {
ValueType valueType = ValueType.lenientParse(p.text());
return valueType;
}, new ParseField("value_type"), ObjectParser.ValueType.STRING);
objectParser.declareField(VB::userValuetypeHint, XContentParser::text, new ParseField("value_type"), ObjectParser.ValueType.STRING);

objectParser.declareField(
VB::script,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ public DateHistogramValuesSourceBuilder offset(long offset) {
public static void register(ValuesSourceRegistry.Builder builder) {
builder.register(
REGISTRY_KEY,
List.of(CoreValuesSourceType.DATE, CoreValuesSourceType.NUMERIC),
List.of(CoreValuesSourceType.DATE, CoreValuesSourceType.DOUBLE, CoreValuesSourceType.LONG),
(valuesSourceConfig, rounding, name, hasScript, format, missingBucket, missingOrder, order) -> {
ValuesSource.Numeric numeric = (ValuesSource.Numeric) valuesSourceConfig.getValuesSource();
// TODO once composite is plugged in to the values source registry or at least understands Date values source types use it
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ static HistogramValuesSourceBuilder parse(String name, XContentParser parser) th
public static void register(ValuesSourceRegistry.Builder builder) {
builder.register(
REGISTRY_KEY,
List.of(CoreValuesSourceType.DATE, CoreValuesSourceType.NUMERIC),
List.of(CoreValuesSourceType.DATE, CoreValuesSourceType.DOUBLE, CoreValuesSourceType.LONG),
(valuesSourceConfig, interval, name, hasScript, format, missingBucket, missingOrder, order) -> {
ValuesSource.Numeric numeric = (ValuesSource.Numeric) valuesSourceConfig.getValuesSource();
final HistogramValuesSource vs = new HistogramValuesSource(numeric, interval);
Expand Down Expand Up @@ -166,7 +166,7 @@ public HistogramValuesSourceBuilder interval(double interval) {

@Override
protected ValuesSourceType getDefaultValuesSourceType() {
return CoreValuesSourceType.NUMERIC;
return CoreValuesSourceType.DOUBLE;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ public String type() {
static void register(ValuesSourceRegistry.Builder builder) {
builder.register(
REGISTRY_KEY,
List.of(CoreValuesSourceType.DATE, CoreValuesSourceType.NUMERIC, CoreValuesSourceType.BOOLEAN),
List.of(CoreValuesSourceType.DATE, CoreValuesSourceType.DOUBLE, CoreValuesSourceType.LONG, CoreValuesSourceType.BOOLEAN),
(valuesSourceConfig, name, hasScript, format, missingBucket, missingOrder, order) -> {
final DocValueFormat docValueFormat;
if (format == null && valuesSourceConfig.valueSourceType() == CoreValuesSourceType.DATE) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public final class AutoDateHistogramAggregatorFactory extends ValuesSourceAggreg
public static void registerAggregators(ValuesSourceRegistry.Builder builder) {
builder.register(
AutoDateHistogramAggregationBuilder.REGISTRY_KEY,
List.of(CoreValuesSourceType.DATE, CoreValuesSourceType.NUMERIC),
List.of(CoreValuesSourceType.DATE, CoreValuesSourceType.DOUBLE, CoreValuesSourceType.LONG),
AutoDateHistogramAggregator::build,
true
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public final class DateHistogramAggregatorFactory extends ValuesSourceAggregator
public static void registerAggregators(ValuesSourceRegistry.Builder builder) {
builder.register(
DateHistogramAggregationBuilder.REGISTRY_KEY,
List.of(CoreValuesSourceType.DATE, CoreValuesSourceType.NUMERIC),
List.of(CoreValuesSourceType.DATE, CoreValuesSourceType.DOUBLE, CoreValuesSourceType.LONG),
DateHistogramAggregator::build,
true
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ public static void registerAggregators(ValuesSourceRegistry.Builder builder) {

@Override
protected ValuesSourceType defaultValueSourceType() {
return CoreValuesSourceType.NUMERIC;
return CoreValuesSourceType.DOUBLE;
}

/** Create a new builder with the given name. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ static void registerAggregators(ValuesSourceRegistry.Builder builder) {

builder.register(
HistogramAggregationBuilder.REGISTRY_KEY,
List.of(CoreValuesSourceType.NUMERIC, CoreValuesSourceType.DATE, CoreValuesSourceType.BOOLEAN),
List.of(CoreValuesSourceType.DOUBLE, CoreValuesSourceType.LONG, CoreValuesSourceType.DATE, CoreValuesSourceType.BOOLEAN),
NumericHistogramAggregator::new,
true
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ protected VariableWidthHistogramAggregationBuilder(

@Override
protected ValuesSourceType defaultValueSourceType() {
return CoreValuesSourceType.NUMERIC;
return CoreValuesSourceType.DOUBLE;
}

public VariableWidthHistogramAggregationBuilder setNumBuckets(int numBuckets) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,15 @@
import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry;

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

public class VariableWidthHistogramAggregatorFactory extends ValuesSourceAggregatorFactory {

public static void registerAggregators(ValuesSourceRegistry.Builder builder) {
builder.register(
VariableWidthHistogramAggregationBuilder.REGISTRY_KEY,
CoreValuesSourceType.NUMERIC,
List.of(CoreValuesSourceType.DOUBLE, CoreValuesSourceType.LONG),
VariableWidthHistogramAggregator::new,
true
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,12 @@ public class DateRangeAggregationBuilder extends AbstractRangeBuilder<DateRangeA
private static final DeprecationLogger DEPRECATION_LOGGER = DeprecationLogger.getLogger(DateRangeAggregationBuilder.class);

public static void registerAggregators(ValuesSourceRegistry.Builder builder) {
builder.register(REGISTRY_KEY, List.of(CoreValuesSourceType.NUMERIC, CoreValuesSourceType.DATE), RangeAggregator::build, true);
builder.register(
REGISTRY_KEY,
List.of(CoreValuesSourceType.DOUBLE, CoreValuesSourceType.LONG, CoreValuesSourceType.DATE),
RangeAggregator::build,
true
);

builder.register(
REGISTRY_KEY,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ public int hashCode() {

public static class Factory<B extends Bucket, R extends InternalRange<B, R>> {
public ValuesSourceType getValueSourceType() {
return CoreValuesSourceType.NUMERIC;
return CoreValuesSourceType.DOUBLE;
}

public ValueType getValueType() {
Expand Down
Loading