Skip to content

Commit

Permalink
Pass script params through to fielddata impl (#59762)
Browse files Browse the repository at this point in the history
This addresses a TODO around using the script params, which are now parsed from the mappings. It also expand existing tests to verify that params are carried around and accessible in script for both fielddata and queries.

Relates to #59332
  • Loading branch information
javanna authored Jul 17, 2020
1 parent 390fa65 commit 45d0dc5
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.indices.breaker.CircuitBreakerService;
import org.elasticsearch.script.Script;
import org.elasticsearch.search.DocValueFormat;
import org.elasticsearch.search.MultiValueMode;
import org.elasticsearch.search.aggregations.support.CoreValuesSourceType;
Expand All @@ -34,7 +35,6 @@

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;

public final class ScriptBinaryFieldData extends AbstractIndexComponent
Expand All @@ -44,9 +44,11 @@ public final class ScriptBinaryFieldData extends AbstractIndexComponent

public static class Builder implements IndexFieldData.Builder {

private final Script script;
private final StringScriptFieldScript.Factory scriptFactory;

public Builder(StringScriptFieldScript.Factory scriptFactory) {
public Builder(Script script, StringScriptFieldScript.Factory scriptFactory) {
this.script = script;
this.scriptFactory = scriptFactory;
}

Expand All @@ -58,23 +60,29 @@ public ScriptBinaryFieldData build(
CircuitBreakerService breakerService,
MapperService mapperService
) {
return new ScriptBinaryFieldData(indexSettings, fieldType.name(), scriptFactory);
return new ScriptBinaryFieldData(indexSettings, fieldType.name(), script, scriptFactory);
}
}

private final String fieldName;
private final Script script;
private final StringScriptFieldScript.Factory scriptFactory;
private final SetOnce<StringScriptFieldScript.LeafFactory> leafFactory = new SetOnce<>();

private ScriptBinaryFieldData(IndexSettings indexSettings, String fieldName, StringScriptFieldScript.Factory scriptFactory) {
private ScriptBinaryFieldData(
IndexSettings indexSettings,
String fieldName,
Script script,
StringScriptFieldScript.Factory scriptFactory
) {
super(indexSettings);
this.fieldName = fieldName;
this.script = script;
this.scriptFactory = scriptFactory;
}

public void setSearchLookup(SearchLookup searchLookup) {
// TODO wire the params from the mappings definition, we don't parse them yet
this.leafFactory.set(scriptFactory.newFactory(Collections.emptyMap(), searchLookup));
this.leafFactory.set(scriptFactory.newFactory(script.getParams(), searchLookup));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public boolean isAggregatable() {
@Override
public ScriptBinaryFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName) {
// TODO once we get SearchLookup as an argument, we can already call scriptFactory.newFactory here and pass through the result
return new ScriptBinaryFieldData.Builder(scriptFactory);
return new ScriptBinaryFieldData.Builder(script, scriptFactory);
}

private StringScriptFieldScript.LeafFactory leafFactory(QueryShardContext context) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,12 @@
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.painless.PainlessPlugin;
import org.elasticsearch.painless.PainlessScriptEngine;
import org.elasticsearch.plugins.ExtensiblePlugin.ExtensionLoader;
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptModule;
import org.elasticsearch.script.ScriptService;
import org.elasticsearch.script.ScriptType;
import org.elasticsearch.search.lookup.SearchLookup;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.runtimefields.RuntimeFields;
Expand All @@ -43,6 +45,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.function.BiConsumer;

import static java.util.Collections.emptyMap;
Expand All @@ -58,7 +61,10 @@ public void testDocValues() throws IOException {
List<String> results = new ArrayList<>();
try (DirectoryReader reader = iw.getReader()) {
IndexSearcher searcher = newSearcher(reader);
RuntimeKeywordMappedFieldType ft = build("for (def v : source.foo) {value(v.toString())}");
RuntimeKeywordMappedFieldType ft = build(
"for (def v : source.foo) {value(v.toString() + params.param)}",
Map.of("param", "-suffix")
);
IndexMetadata imd = IndexMetadata.builder("test")
.settings(Settings.builder().put("index.version.created", Version.CURRENT))
.numberOfShards(1)
Expand All @@ -73,11 +79,11 @@ public ScoreMode scoreMode() {
}

@Override
public LeafCollector getLeafCollector(LeafReaderContext context) throws IOException {
public LeafCollector getLeafCollector(LeafReaderContext context) {
SortedBinaryDocValues dv = ifd.load(context).getBytesValues();
return new LeafCollector() {
@Override
public void setScorer(Scorable scorer) throws IOException {}
public void setScorer(Scorable scorer) {}

@Override
public void collect(int doc) throws IOException {
Expand All @@ -90,7 +96,7 @@ public void collect(int doc) throws IOException {
};
}
});
assertThat(results, equalTo(List.of("1", "1", "2")));
assertThat(results, equalTo(List.of("1-suffix", "1-suffix", "2-suffix")));
}
}
}
Expand Down Expand Up @@ -213,7 +219,8 @@ public void testTermQuery() throws IOException {
iw.addDocument(List.of(new StoredField("_source", new BytesRef("{\"foo\": 2}"))));
try (DirectoryReader reader = iw.getReader()) {
IndexSearcher searcher = newSearcher(reader);
assertThat(searcher.count(build("value(source.foo.toString())").termQuery("1", mockContext())), equalTo(1));
RuntimeKeywordMappedFieldType fieldType = build("value(source.foo.toString() + params.param)", Map.of("param", "-suffix"));
assertThat(searcher.count(fieldType.termQuery("1-suffix", mockContext())), equalTo(1));
}
}
}
Expand Down Expand Up @@ -255,7 +262,14 @@ public void testWildcardQueryIsExpensive() throws IOException {
}

private RuntimeKeywordMappedFieldType build(String code) throws IOException {
Script script = new Script(code);
return build(new Script(code));
}

private RuntimeKeywordMappedFieldType build(String code, Map<String, Object> params) throws IOException {
return build(new Script(ScriptType.INLINE, PainlessScriptEngine.NAME, code, params));
}

private RuntimeKeywordMappedFieldType build(Script script) throws IOException {
PainlessPlugin painlessPlugin = new PainlessPlugin();
painlessPlugin.loadExtensions(new ExtensionLoader() {
@Override
Expand Down

0 comments on commit 45d0dc5

Please sign in to comment.