Skip to content

Check for runtime field loops in queries #61927

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Sep 15, 2020
Merged

Check for runtime field loops in queries #61927

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c39360a
Check for runtime field loops in queries
nik9000 Sep 3, 2020
0716b27
Merge branch 'master' into runtime_fields_loop_in_query
nik9000 Sep 3, 2020
9ce7174
Merge branch 'master' into runtime_fields_loop_in_query
nik9000 Sep 4, 2020
72c1b8b
Merge branch 'master' into runtime_fields_loop_in_query
nik9000 Sep 10, 2020
f780a42
Compile
nik9000 Sep 10, 2020
4bc988b
Prettier
nik9000 Sep 14, 2020
b66a16d
Merge branch 'master' into runtime_fields_loop_in_query
nik9000 Sep 14, 2020
45d124b
iter
nik9000 Sep 15, 2020
bef47e2
Merge branch 'master' into runtime_fields_loop_in_query
nik9000 Sep 15, 2020
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
48 changes: 44 additions & 4 deletions ...main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.apache.lucene.analysis.TokenStream;
import org.apache.lucene.search.MultiTermQuery;
import org.apache.lucene.search.Query;
import org.apache.lucene.search.MultiTermQuery.RewriteMethod;
import org.apache.lucene.search.spans.SpanMultiTermQueryWrapper;
import org.apache.lucene.search.spans.SpanQuery;
import org.elasticsearch.ElasticsearchException;
Expand All @@ -19,24 +20,33 @@
import org.elasticsearch.index.mapper.TextSearchInfo;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.script.Script;
import org.elasticsearch.search.lookup.SearchLookup;

import java.io.IOException;
import java.time.ZoneId;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.function.BiFunction;

import static org.elasticsearch.search.SearchService.ALLOW_EXPENSIVE_QUERIES;

/**
* Abstract base {@linkplain MappedFieldType} for scripted fields.
*/
abstract class AbstractScriptMappedFieldType extends MappedFieldType {
abstract class AbstractScriptMappedFieldType<LeafFactory> extends MappedFieldType {
protected final Script script;
private final BiFunction<Map<String, Object>, SearchLookup, LeafFactory> factory;

AbstractScriptMappedFieldType(String name, Script script, Map<String, String> meta) {
AbstractScriptMappedFieldType(
String name,
Script script,
BiFunction<Map<String, Object>, SearchLookup, LeafFactory> factory,
Map<String, String> meta
) {
super(name, false, false, TextSearchInfo.SIMPLE_MATCH_ONLY, meta);
this.script = script;
this.factory = factory;
}

protected abstract String runtimeType();
Expand All @@ -61,6 +71,26 @@ public final boolean isAggregatable() {
return true;
}

/**
Copy link
Member Author

Choose a reason for hiding this comment

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

I pulled this function into the superclass because it the "for query" version of it is tricky. We could probably avoid the type parameter if we really wanted to by being a bit more tricky, but I'm not sure it is worth it.

* Create a script leaf factory.
*/
protected final LeafFactory leafFactory(SearchLookup searchLookup) {
return factory.apply(script.getParams(), searchLookup);
}

/**
* Create a script leaf factory for queries.
*/
protected final LeafFactory leafFactory(QueryShardContext context) {
/*
* Forking here causes us to count this field in the field data loop
* detection code as though we were resolving field data for this field.
* We're not, but running the query is close enough.
*/
return leafFactory(context.lookup().forkAndTrackFieldReferences(name()));
}

@Override
public abstract Query termsQuery(List<?> values, QueryShardContext context);

@Override
Expand Down Expand Up @@ -91,6 +121,7 @@ protected abstract Query rangeQuery(
QueryShardContext context
);

@Override
public Query fuzzyQuery(
Object value,
Fuzziness fuzziness,
Expand All @@ -102,38 +133,47 @@ public Query fuzzyQuery(
throw new IllegalArgumentException(unsupported("fuzzy", "keyword and text"));
}

@Override
public Query prefixQuery(String value, MultiTermQuery.RewriteMethod method, QueryShardContext context) {
throw new IllegalArgumentException(unsupported("prefix", "keyword, text and wildcard"));
}

@Override
public Query wildcardQuery(String value, MultiTermQuery.RewriteMethod method, QueryShardContext context) {
throw new IllegalArgumentException(unsupported("wildcard", "keyword, text and wildcard"));
}

@Override
public Query regexpQuery(
String value,
int flags,
int syntaxFlags,
int matchFlags,
Copy link
Member Author

Choose a reason for hiding this comment

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

Missing this was a bug that causes regexes not to work at all!

int maxDeterminizedStates,
MultiTermQuery.RewriteMethod method,
RewriteMethod method,
QueryShardContext context
) {
throw new IllegalArgumentException(unsupported("regexp", "keyword and text"));
}

@Override
public abstract Query existsQuery(QueryShardContext context);

@Override
public Query phraseQuery(TokenStream stream, int slop, boolean enablePositionIncrements) throws IOException {
throw new IllegalArgumentException(unsupported("phrase", "text"));
}

@Override
public Query multiPhraseQuery(TokenStream stream, int slop, boolean enablePositionIncrements) throws IOException {
throw new IllegalArgumentException(unsupported("phrase", "text"));
}

@Override
public Query phrasePrefixQuery(TokenStream stream, int slop, int maxExpansions) throws IOException {
throw new IllegalArgumentException(unsupported("phrase prefix", "text"));
}

@Override
Copy link
Member

Choose a reason for hiding this comment

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

I've seen this popup in this and some other PR, can we merge this fix separately?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've pushed a fix for this to master and 7.x. I'll merge that into this branch now.

public SpanQuery spanPrefixQuery(String value, SpanMultiTermQueryWrapper.SpanRewriteMethod method, QueryShardContext context) {
throw new IllegalArgumentException(unsupported("span prefix", "text"));
}
Expand Down
10 changes: 5 additions & 5 deletions .../src/main/java/org/elasticsearch/xpack/runtimefields/mapper/RuntimeScriptFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public <FactoryType> FactoryType compile(Script script, ScriptContext<FactoryTyp

protected RuntimeScriptFieldMapper(
String simpleName,
AbstractScriptMappedFieldType mappedFieldType,
AbstractScriptMappedFieldType<?> mappedFieldType,
MultiFields multiFields,
CopyTo copyTo,
String runtimeType,
Expand Down Expand Up @@ -86,7 +86,7 @@ protected String contentType() {

public static class Builder extends ParametrizedFieldMapper.Builder {

static final Map<String, BiFunction<Builder, BuilderContext, AbstractScriptMappedFieldType>> FIELD_TYPE_RESOLVER = Map.of(
static final Map<String, BiFunction<Builder, BuilderContext, AbstractScriptMappedFieldType<?>>> FIELD_TYPE_RESOLVER = Map.of(
BooleanFieldMapper.CONTENT_TYPE,
(builder, context) -> {
builder.formatAndLocaleNotSupported();
Expand Down Expand Up @@ -211,7 +211,7 @@ private static RuntimeScriptFieldMapper toType(FieldMapper in) {
private final Parameter<String> format = Parameter.stringParam(
"format",
true,
mapper -> ((AbstractScriptMappedFieldType) mapper.fieldType()).format(),
mapper -> ((AbstractScriptMappedFieldType<?>) mapper.fieldType()).format(),
null
).setSerializer((b, n, v) -> {
if (v != null && false == v.equals(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.pattern())) {
Expand All @@ -223,7 +223,7 @@ private static RuntimeScriptFieldMapper toType(FieldMapper in) {
true,
() -> null,
(n, c, o) -> o == null ? null : LocaleUtils.parse(o.toString()),
mapper -> ((AbstractScriptMappedFieldType) mapper.fieldType()).formatLocale()
mapper -> ((AbstractScriptMappedFieldType<?>) mapper.fieldType()).formatLocale()
).setSerializer((b, n, v) -> {
if (v != null && false == v.equals(Locale.ROOT)) {
b.field(n, v.toString());
Expand All @@ -244,7 +244,7 @@ protected List<Parameter<?>> getParameters() {

@Override
public RuntimeScriptFieldMapper build(BuilderContext context) {
BiFunction<Builder, BuilderContext, AbstractScriptMappedFieldType> fieldTypeResolver = Builder.FIELD_TYPE_RESOLVER.get(
BiFunction<Builder, BuilderContext, AbstractScriptMappedFieldType<?>> fieldTypeResolver = Builder.FIELD_TYPE_RESOLVER.get(
runtimeType.getValue()
);
if (fieldTypeResolver == null) {
Expand Down
19 changes: 6 additions & 13 deletions .../main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptBooleanMappedFieldType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,9 @@
import java.util.Map;
import java.util.function.Supplier;

public class ScriptBooleanMappedFieldType extends AbstractScriptMappedFieldType {
private final BooleanScriptFieldScript.Factory scriptFactory;

public class ScriptBooleanMappedFieldType extends AbstractScriptMappedFieldType<BooleanScriptFieldScript.LeafFactory> {
ScriptBooleanMappedFieldType(String name, Script script, BooleanScriptFieldScript.Factory scriptFactory, Map<String, String> meta) {
super(name, script, meta);
this.scriptFactory = scriptFactory;
super(name, script, scriptFactory::newFactory, meta);
}

@Override
Expand Down Expand Up @@ -71,14 +68,10 @@ public ScriptBooleanFieldData.Builder fielddataBuilder(String fullyQualifiedInde
return new ScriptBooleanFieldData.Builder(name(), leafFactory(searchLookup.get()));
}

private BooleanScriptFieldScript.LeafFactory leafFactory(SearchLookup searchLookup) {
return scriptFactory.newFactory(script.getParams(), searchLookup);
}

@Override
public Query existsQuery(QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new BooleanScriptFieldExistsQuery(script, leafFactory(context.lookup()), name());
return new BooleanScriptFieldExistsQuery(script, leafFactory(context), name());
}

@Override
Expand Down Expand Up @@ -149,7 +142,7 @@ public Query rangeQuery(
@Override
public Query termQuery(Object value, QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new BooleanScriptFieldTermQuery(script, leafFactory(context.lookup()), name(), toBoolean(value));
return new BooleanScriptFieldTermQuery(script, leafFactory(context), name(), toBoolean(value));
}

@Override
Expand All @@ -176,11 +169,11 @@ private Query termsQuery(boolean trueAllowed, boolean falseAllowed, QueryShardCo
return existsQuery(context);
}
checkAllowExpensiveQueries(context);
return new BooleanScriptFieldTermQuery(script, leafFactory(context.lookup()), name(), true);
return new BooleanScriptFieldTermQuery(script, leafFactory(context), name(), true);
}
if (falseAllowed) {
checkAllowExpensiveQueries(context);
return new BooleanScriptFieldTermQuery(script, leafFactory(context.lookup()), name(), false);
return new BooleanScriptFieldTermQuery(script, leafFactory(context), name(), false);
}
return new MatchNoDocsQuery("neither true nor false allowed");
}
Expand Down
20 changes: 7 additions & 13 deletions ...src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptDateMappedFieldType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@
import java.util.Map;
import java.util.function.Supplier;

public class ScriptDateMappedFieldType extends AbstractScriptMappedFieldType {
private final DateScriptFieldScript.Factory scriptFactory;
public class ScriptDateMappedFieldType extends AbstractScriptMappedFieldType<DateScriptFieldScript.LeafFactory> {
private final DateFormatter dateTimeFormatter;

ScriptDateMappedFieldType(
Expand All @@ -48,8 +47,7 @@ public class ScriptDateMappedFieldType extends AbstractScriptMappedFieldType {
DateFormatter dateTimeFormatter,
Map<String, String> meta
) {
super(name, script, meta);
this.scriptFactory = scriptFactory;
super(name, script, (params, ctx) -> scriptFactory.newFactory(params, ctx, dateTimeFormatter), meta);
this.dateTimeFormatter = dateTimeFormatter;
}

Expand Down Expand Up @@ -84,10 +82,6 @@ public ScriptDateFieldData.Builder fielddataBuilder(String fullyQualifiedIndexNa
return new ScriptDateFieldData.Builder(name(), leafFactory(lookup.get()));
}

private DateScriptFieldScript.LeafFactory leafFactory(SearchLookup lookup) {
return scriptFactory.newFactory(script.getParams(), lookup, dateTimeFormatter);
}

@Override
public Query distanceFeatureQuery(Object origin, String pivot, float boost, QueryShardContext context) {
checkAllowExpensiveQueries(context);
Expand All @@ -103,7 +97,7 @@ public Query distanceFeatureQuery(Object origin, String pivot, float boost, Quer
TimeValue pivotTime = TimeValue.parseTimeValue(pivot, "distance_feature.pivot");
return new LongScriptFieldDistanceFeatureQuery(
script,
leafFactory(context.lookup())::newInstance,
leafFactory(context)::newInstance,
name(),
originLong,
pivotTime.getMillis(),
Expand All @@ -115,7 +109,7 @@ public Query distanceFeatureQuery(Object origin, String pivot, float boost, Quer
@Override
public Query existsQuery(QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new LongScriptFieldExistsQuery(script, leafFactory(context.lookup())::newInstance, name());
return new LongScriptFieldExistsQuery(script, leafFactory(context)::newInstance, name());
}

@Override
Expand All @@ -139,7 +133,7 @@ public Query rangeQuery(
parser,
context,
DateFieldMapper.Resolution.MILLISECONDS,
(l, u) -> new LongScriptFieldRangeQuery(script, leafFactory(context.lookup())::newInstance, name(), l, u)
(l, u) -> new LongScriptFieldRangeQuery(script, leafFactory(context)::newInstance, name(), l, u)
);
}

Expand All @@ -155,7 +149,7 @@ public Query termQuery(Object value, QueryShardContext context) {
DateFieldMapper.Resolution.MILLISECONDS
);
checkAllowExpensiveQueries(context);
return new LongScriptFieldTermQuery(script, leafFactory(context.lookup())::newInstance, name(), l);
return new LongScriptFieldTermQuery(script, leafFactory(context)::newInstance, name(), l);
});
}

Expand All @@ -179,7 +173,7 @@ public Query termsQuery(List<?> values, QueryShardContext context) {
);
}
checkAllowExpensiveQueries(context);
return new LongScriptFieldTermsQuery(script, leafFactory(context.lookup())::newInstance, name(), terms);
return new LongScriptFieldTermsQuery(script, leafFactory(context)::newInstance, name(), terms);
});
}

Expand Down
19 changes: 6 additions & 13 deletions ...c/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptDoubleMappedFieldType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,9 @@
import java.util.Map;
import java.util.function.Supplier;

public class ScriptDoubleMappedFieldType extends AbstractScriptMappedFieldType {
private final DoubleScriptFieldScript.Factory scriptFactory;

public class ScriptDoubleMappedFieldType extends AbstractScriptMappedFieldType<DoubleScriptFieldScript.LeafFactory> {
ScriptDoubleMappedFieldType(String name, Script script, DoubleScriptFieldScript.Factory scriptFactory, Map<String, String> meta) {
super(name, script, meta);
this.scriptFactory = scriptFactory;
super(name, script, scriptFactory::newFactory, meta);
}

@Override
Expand Down Expand Up @@ -63,14 +60,10 @@ public ScriptDoubleFieldData.Builder fielddataBuilder(String fullyQualifiedIndex
return new ScriptDoubleFieldData.Builder(name(), leafFactory(searchLookup.get()));
}

private DoubleScriptFieldScript.LeafFactory leafFactory(SearchLookup searchLookup) {
return scriptFactory.newFactory(script.getParams(), searchLookup);
}

@Override
public Query existsQuery(QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new DoubleScriptFieldExistsQuery(script, leafFactory(context.lookup()), name());
return new DoubleScriptFieldExistsQuery(script, leafFactory(context), name());
}

@Override
Expand All @@ -89,14 +82,14 @@ public Query rangeQuery(
upperTerm,
includeLower,
includeUpper,
(l, u) -> new DoubleScriptFieldRangeQuery(script, leafFactory(context.lookup()), name(), l, u)
(l, u) -> new DoubleScriptFieldRangeQuery(script, leafFactory(context), name(), l, u)
);
}

@Override
public Query termQuery(Object value, QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new DoubleScriptFieldTermQuery(script, leafFactory(context.lookup()), name(), NumberType.objectToDouble(value));
return new DoubleScriptFieldTermQuery(script, leafFactory(context), name(), NumberType.objectToDouble(value));
}

@Override
Expand All @@ -109,6 +102,6 @@ public Query termsQuery(List<?> values, QueryShardContext context) {
terms.add(Double.doubleToLongBits(NumberType.objectToDouble(value)));
}
checkAllowExpensiveQueries(context);
return new DoubleScriptFieldTermsQuery(script, leafFactory(context.lookup()), name(), terms);
return new DoubleScriptFieldTermsQuery(script, leafFactory(context), name(), terms);
}
}
Loading