Make ScriptFieldMapper a parameterized mapper #59391
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,42 +6,54 @@ | |
|
|
||
| package org.elasticsearch.xpack.runtimefields.mapper; | ||
|
|
||
| import org.apache.lucene.util.SetOnce; | ||
| import org.elasticsearch.common.xcontent.XContentBuilder; | ||
| import org.elasticsearch.index.mapper.FieldMapper; | ||
| import org.elasticsearch.index.mapper.MappedFieldType; | ||
| import org.elasticsearch.index.mapper.Mapper; | ||
| import org.elasticsearch.index.mapper.MapperParsingException; | ||
| import org.elasticsearch.index.mapper.ParametrizedFieldMapper; | ||
| import org.elasticsearch.index.mapper.ParseContext; | ||
| import org.elasticsearch.script.Script; | ||
| import org.elasticsearch.script.ScriptService; | ||
| import org.elasticsearch.script.ScriptType; | ||
| import org.elasticsearch.xpack.runtimefields.StringScriptFieldScript; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public final class ScriptFieldMapper extends ParametrizedFieldMapper { | ||
|
|
||
| public static final String CONTENT_TYPE = "script"; | ||
|
|
||
| private final String runtimeType; | ||
| private final Script script; | ||
| private final ScriptService scriptService; | ||
|
|
||
| protected ScriptFieldMapper( | ||
| String simpleName, | ||
| MappedFieldType mappedFieldType, | ||
| MultiFields multiFields, | ||
| CopyTo copyTo, | ||
| String runtimeType, | ||
| Script script, | ||
| ScriptService scriptService | ||
| ) { | ||
| super(simpleName, mappedFieldType, multiFields, copyTo); | ||
| this.runtimeType = runtimeType; | ||
| this.script = script; | ||
| this.scriptService = scriptService; | ||
| } | ||
|
|
||
| @Override | ||
| public ParametrizedFieldMapper.Builder getMergeBuilder() { | ||
| return new ScriptFieldMapper.Builder(simpleName(), scriptService).init(this); | ||
| } | ||
|
|
||
| @Override | ||
| protected void parseCreateField(ParseContext context) { | ||
| // there is no lucene field | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -56,44 +68,76 @@ protected String contentType() { | |
| return CONTENT_TYPE; | ||
| } | ||
|
|
||
| public static class Builder extends ParametrizedFieldMapper.Builder { | ||
|
|
||
| private static ScriptFieldMapper toType(FieldMapper in) { | ||
| return (ScriptFieldMapper) in; | ||
| } | ||
|
|
||
| private final Parameter<Map<String, String>> meta = Parameter.metaParam(); | ||
| private final Parameter<String> runtimeType = Parameter.stringParam( | ||
| "runtime_type", | ||
| true, | ||
| mapper -> toType(mapper).runtimeType, | ||
| null | ||
| ).setValidator(runtimeType -> { | ||
| if (runtimeType == null) { | ||
| throw new IllegalArgumentException("runtime_type must be specified for script field [" + name + "]"); | ||
| } | ||
| }); | ||
| // TODO script and runtime_type can be updated: what happens to the currently running queries when they get updated? | ||
| // do all the shards get a consistent view? | ||
| private final Parameter<Script> script = new Parameter<>( | ||
| "script", | ||
| true, | ||
| null, | ||
| Builder::parseScript, | ||
| mapper -> toType(mapper).script | ||
| ).setValidator(script -> { | ||
| if (script == null) { | ||
| throw new IllegalArgumentException("script must be specified for script field [" + name + "]"); | ||
| } | ||
| }); | ||
|
|
||
| private final ScriptService scriptService; | ||
|
|
||
| protected Builder(String name, ScriptService scriptService) { | ||
| super(name); | ||
| this.scriptService = scriptService; | ||
| } | ||
|
|
||
| @Override | ||
| protected List<Parameter<?>> getParameters() { | ||
| return List.of(meta, runtimeType, script); | ||
| } | ||
|
|
||
| @Override | ||
| public ScriptFieldMapper build(BuilderContext context) { | ||
| MappedFieldType mappedFieldType; | ||
| if (runtimeType.getValue().equals("keyword")) { | ||
| StringScriptFieldScript.Factory factory = scriptService.compile(script.getValue(), StringScriptFieldScript.CONTEXT); | ||
| mappedFieldType = new RuntimeKeywordMappedFieldType(buildFullName(context), script.getValue(), factory, meta.getValue()); | ||
| } else { | ||
| throw new IllegalArgumentException("runtime_type [" + runtimeType + "] not supported"); | ||
| } | ||
| // TODO copy to and multi_fields should not be supported, parametrized field mapper needs to be adapted | ||
|
There was a problem hiding this comment. @romseygeek and this one too. I think I would prefer making the changes upstream instead of adapting parameterized field mapper in the feature branch. Thoughts? There was a problem hiding this comment. ++, I can work on this one. There was a problem hiding this comment. thanks that would be great! |
||
| return new ScriptFieldMapper( | ||
| name, | ||
| mappedFieldType, | ||
| multiFieldsBuilder.build(this, context), | ||
| copyTo.build(), | ||
| runtimeType.getValue(), | ||
| script.getValue(), | ||
| scriptService | ||
| ); | ||
| } | ||
|
|
||
| static Script parseScript(String name, Mapper.TypeParser.ParserContext parserContext, Object scriptObject) { | ||
|
There was a problem hiding this comment. I swear I've seen this before somewhere else... There was a problem hiding this comment. you may have, but we only accept painless and we don't parse stored scripts. I will look though, I assumed that we never parse from a map but watcher may do that actually. There was a problem hiding this comment. I found it, it was in update by query, I opened #59507 so that we can test and reuse that existing code |
||
| Script script = Script.parse(scriptObject); | ||
| if (script.getType() == ScriptType.STORED) { | ||
| throw new IllegalArgumentException("stored scripts specified but not supported when defining script field [" + name + "]"); | ||
| } | ||
| return script; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -108,29 +152,9 @@ public void setScriptService(ScriptService scriptService) { | |
| @Override | ||
| public ScriptFieldMapper.Builder parse(String name, Map<String, Object> node, ParserContext parserContext) | ||
| throws MapperParsingException { | ||
|
|
||
| ScriptFieldMapper.Builder builder = new ScriptFieldMapper.Builder(name, scriptService.get()); | ||
| builder.parse(name, parserContext, node); | ||
| return builder; | ||
| } | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License; | ||
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
|
|
||
| package org.elasticsearch.xpack.runtimefields.mapper; | ||
|
|
||
| import org.elasticsearch.common.Strings; | ||
| import org.elasticsearch.common.settings.Settings; | ||
| import org.elasticsearch.common.xcontent.XContentBuilder; | ||
| import org.elasticsearch.common.xcontent.XContentFactory; | ||
| import org.elasticsearch.index.mapper.FieldMapper; | ||
| import org.elasticsearch.index.mapper.MapperParsingException; | ||
| import org.elasticsearch.index.mapper.MapperService; | ||
| import org.elasticsearch.plugins.Plugin; | ||
| import org.elasticsearch.plugins.ScriptPlugin; | ||
| import org.elasticsearch.script.ScriptContext; | ||
| import org.elasticsearch.script.ScriptEngine; | ||
| import org.elasticsearch.test.ESSingleNodeTestCase; | ||
| import org.elasticsearch.test.InternalSettingsPlugin; | ||
| import org.elasticsearch.xpack.runtimefields.RuntimeFields; | ||
| import org.elasticsearch.xpack.runtimefields.StringScriptFieldScript; | ||
|
|
||
| import java.util.Collection; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
|
|
||
| import static org.hamcrest.Matchers.instanceOf; | ||
|
|
||
| public class ScriptFieldMapperTests extends ESSingleNodeTestCase { | ||
|
|
||
| private static final String[] SUPPORTED_RUNTIME_TYPES = new String[] { "keyword" }; | ||
|
|
||
| @Override | ||
| protected Collection<Class<? extends Plugin>> getPlugins() { | ||
| return pluginList(InternalSettingsPlugin.class, RuntimeFields.class, TestScriptPlugin.class); | ||
| } | ||
|
|
||
| public void testRuntimeTypeIsRequired() throws Exception { | ||
| XContentBuilder mapping = XContentFactory.jsonBuilder() | ||
| .startObject() | ||
| .startObject("_doc") | ||
| .startObject("properties") | ||
| .startObject("my_field") | ||
| .field("type", "script") | ||
| .field("script", "value('test')") | ||
| .endObject() | ||
| .endObject() | ||
| .endObject() | ||
| .endObject(); | ||
|
|
||
| MapperParsingException exception = expectThrows(MapperParsingException.class, () -> createIndex("test", Settings.EMPTY, mapping)); | ||
| assertEquals("Failed to parse mapping: runtime_type must be specified for script field [my_field]", exception.getMessage()); | ||
| } | ||
|
|
||
| public void testScriptIsRequired() throws Exception { | ||
| XContentBuilder mapping = XContentFactory.jsonBuilder() | ||
| .startObject() | ||
| .startObject("_doc") | ||
| .startObject("properties") | ||
| .startObject("my_field") | ||
| .field("type", "script") | ||
| .field("runtime_type", randomFrom(SUPPORTED_RUNTIME_TYPES)) | ||
| .endObject() | ||
| .endObject() | ||
| .endObject() | ||
| .endObject(); | ||
|
|
||
| MapperParsingException exception = expectThrows(MapperParsingException.class, () -> createIndex("test", Settings.EMPTY, mapping)); | ||
| assertEquals("Failed to parse mapping: script must be specified for script field [my_field]", exception.getMessage()); | ||
| } | ||
|
|
||
| public void testStoredScriptsAreNotSupported() throws Exception { | ||
| XContentBuilder mapping = XContentFactory.jsonBuilder() | ||
| .startObject() | ||
| .startObject("_doc") | ||
| .startObject("properties") | ||
| .startObject("my_field") | ||
| .field("type", "script") | ||
| .field("runtime_type", randomFrom(SUPPORTED_RUNTIME_TYPES)) | ||
| .startObject("script") | ||
| .field("id", "test") | ||
| .endObject() | ||
| .endObject() | ||
| .endObject() | ||
| .endObject() | ||
| .endObject(); | ||
| MapperParsingException exception = expectThrows(MapperParsingException.class, () -> createIndex("test", Settings.EMPTY, mapping)); | ||
| assertEquals( | ||
| "Failed to parse mapping: stored scripts specified but not supported when defining script field [my_field]", | ||
| exception.getMessage() | ||
| ); | ||
| } | ||
|
|
||
| public void testDefaultMapping() throws Exception { | ||
|
There was a problem hiding this comment. Is the idea to test that we can properly parse a simple version of the mapper? I don't really get the name of the method. I will try and hack on it locally and let you know what I find. There was a problem hiding this comment. the name is from a copy/paste. I wanted to test that the mapper works indeed. |
||
| XContentBuilder mapping = XContentFactory.jsonBuilder() | ||
| .startObject() | ||
| .startObject("_doc") | ||
| .startObject("properties") | ||
| .startObject("field") | ||
| .field("type", "script") | ||
| .field("runtime_type", randomFrom(SUPPORTED_RUNTIME_TYPES)) | ||
| .startObject("script") | ||
| .field("source", "value('test')") | ||
| .field("lang", "test") | ||
| .endObject() | ||
| .endObject() | ||
| .endObject() | ||
| .endObject() | ||
| .endObject(); | ||
|
|
||
| MapperService mapperService = createIndex("test", Settings.EMPTY, mapping).mapperService(); | ||
| FieldMapper mapper = (FieldMapper) mapperService.documentMapper().mappers().getMapper("field"); | ||
| assertThat(mapper, instanceOf(ScriptFieldMapper.class)); | ||
| assertEquals(Strings.toString(mapping), Strings.toString(mapperService.documentMapper())); | ||
| } | ||
|
|
||
| public static class TestScriptPlugin extends Plugin implements ScriptPlugin { | ||
| @Override | ||
| public ScriptEngine getScriptEngine(Settings settings, Collection<ScriptContext<?>> contexts) { | ||
| return new ScriptEngine() { | ||
| @Override | ||
| public String getType() { | ||
| return "test"; | ||
| } | ||
|
|
||
| @Override | ||
| public <FactoryType> FactoryType compile( | ||
| String name, | ||
| String code, | ||
| ScriptContext<FactoryType> context, | ||
| Map<String, String> paramsMap | ||
| ) { | ||
| if ("value('test')".equals(code)) { | ||
| StringScriptFieldScript.Factory factory = (params, lookup) -> ctx -> new StringScriptFieldScript( | ||
| params, | ||
| lookup, | ||
| ctx | ||
| ) { | ||
| @Override | ||
| public void execute() { | ||
| this.results.add("test"); | ||
| } | ||
| }; | ||
| @SuppressWarnings("unchecked") | ||
| FactoryType castFactory = (FactoryType) factory; | ||
| return castFactory; | ||
| } | ||
| throw new IllegalArgumentException("No test script for [" + code + "]"); | ||
| } | ||
|
|
||
| @Override | ||
| public Set<ScriptContext<?>> getSupportedContexts() { | ||
| return Set.of(StringScriptFieldScript.CONTEXT); | ||
| } | ||
| }; | ||
| } | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@romseygeek do you have thoughts on this?
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.
So currently
QueryShardContext#fieldMapper()delegates field lookups back to the MapperService, which gets updated in-place, so if there's an update while a query is running then the script will get changed out and there may well be inconsistencies between phases, as well as between shards. We could make QueryShardContext take a FieldTypeLookup as a parameter which would at least remove the possibility of an update between phases?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.
We have this sort of problem now with any thing that can change. I guess scripts make this worse because they are "more mutable" than other stuff.