-
Notifications
You must be signed in to change notification settings - Fork 25.2k
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.
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
Changes from 5 commits
c39360a
0716b27
9ce7174
72c1b8b
f780a42
4bc988b
b66a16d
45d124b
bef47e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,17 +8,20 @@ | |
|
||
import org.apache.lucene.analysis.TokenStream; | ||
import org.apache.lucene.search.MultiTermQuery; | ||
import org.apache.lucene.search.MultiTermQuery.RewriteMethod; | ||
import org.apache.lucene.search.Query; | ||
import org.apache.lucene.search.spans.SpanMultiTermQueryWrapper; | ||
import org.apache.lucene.search.spans.SpanQuery; | ||
import org.elasticsearch.ElasticsearchException; | ||
import org.elasticsearch.common.TriFunction; | ||
import org.elasticsearch.common.geo.ShapeRelation; | ||
import org.elasticsearch.common.time.DateMathParser; | ||
import org.elasticsearch.common.unit.Fuzziness; | ||
import org.elasticsearch.index.mapper.MappedFieldType; | ||
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; | ||
|
@@ -31,12 +34,19 @@ | |
/** | ||
* Abstract base {@linkplain MappedFieldType} for scripted fields. | ||
*/ | ||
abstract class AbstractScriptMappedFieldType extends MappedFieldType { | ||
abstract class AbstractScriptMappedFieldType<LeafFactory> extends MappedFieldType { | ||
protected final Script script; | ||
private final TriFunction<String, Map<String, Object>, SearchLookup, LeafFactory> factory; | ||
|
||
AbstractScriptMappedFieldType(String name, Script script, Map<String, String> meta) { | ||
AbstractScriptMappedFieldType( | ||
String name, | ||
Script script, | ||
TriFunction<String, 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(); | ||
|
@@ -61,6 +71,26 @@ public final boolean isAggregatable() { | |
return true; | ||
} | ||
|
||
/** | ||
* Create a script leaf factory. | ||
*/ | ||
protected final LeafFactory leafFactory(SearchLookup searchLookup) { | ||
return factory.apply(name(), 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 | ||
|
@@ -119,7 +149,7 @@ public Query regexpQuery( | |
int syntaxFlags, | ||
int matchFlags, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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")); | ||
|
@@ -149,7 +179,7 @@ public SpanQuery spanPrefixQuery(String value, SpanMultiTermQueryWrapper.SpanRew | |
} | ||
|
||
private String unsupported(String query, String supported) { | ||
String thisField = "[" + name() + "] which is of type [script] with runtime_type [" + runtimeType() + "]"; | ||
String thisField = "[" + name() + "] which is of type [runtime] with runtime_type [" + runtimeType() + "]"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is something I bumped into while working on this. Just had the old name left over. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use the constant, then renaming is much easier. |
||
return "Can only use " + query + " queries on " + supported + " fields - not on " + thisField; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,15 +37,9 @@ | |
import java.util.Map; | ||
import java.util.function.Supplier; | ||
|
||
public final class ScriptIpMappedFieldType extends AbstractScriptMappedFieldType { | ||
|
||
private final Script script; | ||
private final IpScriptFieldScript.Factory scriptFactory; | ||
|
||
public final class ScriptIpMappedFieldType extends AbstractScriptMappedFieldType<IpScriptFieldScript.LeafFactory> { | ||
ScriptIpMappedFieldType(String name, Script script, IpScriptFieldScript.Factory scriptFactory, Map<String, String> meta) { | ||
super(name, script, meta); | ||
this.script = script; | ||
this.scriptFactory = scriptFactory; | ||
super(name, script, scriptFactory::newFactory, meta); | ||
} | ||
|
||
@Override | ||
|
@@ -79,14 +73,10 @@ public ScriptIpFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName | |
return new ScriptIpFieldData.Builder(name(), leafFactory(searchLookup.get())); | ||
} | ||
|
||
private IpScriptFieldScript.LeafFactory leafFactory(SearchLookup searchLookup) { | ||
return scriptFactory.newFactory(name(), script.getParams(), searchLookup); | ||
} | ||
|
||
@Override | ||
public Query existsQuery(QueryShardContext context) { | ||
checkAllowExpensiveQueries(context); | ||
return new IpScriptFieldExistsQuery(script, leafFactory(context.lookup()), name()); | ||
return new IpScriptFieldExistsQuery(script, leafFactory(context), name()); | ||
} | ||
|
||
@Override | ||
|
@@ -107,7 +97,7 @@ public Query rangeQuery( | |
includeUpper, | ||
(lower, upper) -> new IpScriptFieldRangeQuery( | ||
script, | ||
leafFactory(context.lookup()), | ||
leafFactory(context), | ||
name(), | ||
new BytesRef(InetAddressPoint.encode(lower)), | ||
new BytesRef(InetAddressPoint.encode(upper)) | ||
|
@@ -119,14 +109,18 @@ public Query rangeQuery( | |
public Query termQuery(Object value, QueryShardContext context) { | ||
checkAllowExpensiveQueries(context); | ||
if (value instanceof InetAddress) { | ||
return InetAddressPoint.newExactQuery(name(), (InetAddress) value); | ||
return inetAddressQuery((InetAddress) value, context); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a bug! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we push this separately? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok pushing it as part of this because this is ready. |
||
} | ||
String term = BytesRefs.toString(value); | ||
if (term.contains("/")) { | ||
return cidrQuery(term, context); | ||
} | ||
InetAddress address = InetAddresses.forString(term); | ||
return new IpScriptFieldTermQuery(script, leafFactory(context.lookup()), name(), new BytesRef(InetAddressPoint.encode(address))); | ||
return inetAddressQuery(address, context); | ||
} | ||
|
||
private Query inetAddressQuery(InetAddress address, QueryShardContext context) { | ||
return new IpScriptFieldTermQuery(script, leafFactory(context), name(), new BytesRef(InetAddressPoint.encode(address))); | ||
} | ||
|
||
@Override | ||
|
@@ -149,7 +143,7 @@ public Query termsQuery(List<?> values, QueryShardContext context) { | |
} | ||
cidrQueries.add(cidrQuery(term, context)); | ||
} | ||
Query termsQuery = new IpScriptFieldTermsQuery(script, leafFactory(context.lookup()), name(), terms); | ||
Query termsQuery = new IpScriptFieldTermsQuery(script, leafFactory(context), name(), terms); | ||
if (cidrQueries == null) { | ||
return termsQuery; | ||
} | ||
|
@@ -176,6 +170,6 @@ private Query cidrQuery(String term, QueryShardContext context) { | |
// Force the terms into IPv6 | ||
BytesRef lowerBytes = new BytesRef(InetAddressPoint.encode(InetAddressPoint.decode(lower))); | ||
BytesRef upperBytes = new BytesRef(InetAddressPoint.encode(InetAddressPoint.decode(upper))); | ||
return new IpScriptFieldRangeQuery(script, leafFactory(context.lookup()), name(), lowerBytes, upperBytes); | ||
return new IpScriptFieldRangeQuery(script, leafFactory(context), name(), lowerBytes, upperBytes); | ||
} | ||
} |
There was a problem hiding this comment.
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.