-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Add support for script to boolean field mapper #71454
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 2 commits
f6346d5
ec26992
6df4380
2c42e95
e97452f
55511d5
34d4353
fd81169
e759f87
ab103a7
38d2203
30ef85e
7fd4e4c
142623f
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 |
|---|---|---|
|
|
@@ -293,7 +293,7 @@ public void newDynamicDoubleField(ParseContext context, String name) throws IOEx | |
|
|
||
| @Override | ||
| public void newDynamicBooleanField(ParseContext context, String name) throws IOException { | ||
| createDynamicField(new BooleanFieldMapper.Builder(name), context); | ||
| createDynamicField(new BooleanFieldMapper.Builder(name, null), context); | ||
|
||
| } | ||
|
|
||
| @Override | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,6 +59,8 @@ public abstract class FieldMapper extends Mapper implements Cloneable { | |
| protected final Map<String, NamedAnalyzer> indexAnalyzers; | ||
| protected final MultiFields multiFields; | ||
| protected final CopyTo copyTo; | ||
| protected final boolean hasScript; | ||
| protected final String onScriptError; | ||
|
|
||
| /** | ||
| * Create a FieldMapper with no index analyzers | ||
|
|
@@ -69,9 +71,25 @@ public abstract class FieldMapper extends Mapper implements Cloneable { | |
| */ | ||
| protected FieldMapper(String simpleName, MappedFieldType mappedFieldType, | ||
| MultiFields multiFields, CopyTo copyTo) { | ||
| this(simpleName, mappedFieldType, Collections.emptyMap(), multiFields, copyTo); | ||
| this(simpleName, mappedFieldType, Collections.emptyMap(), multiFields, copyTo, false, null); | ||
| } | ||
|
|
||
| /** | ||
| * Create a FieldMapper with no index analyzers | ||
| * @param simpleName the leaf name of the mapper | ||
| * @param mappedFieldType the MappedFieldType associated with this mapper | ||
| * @param multiFields sub fields of this mapper | ||
| * @param copyTo copyTo fields of this mapper | ||
| * @param hasScript whether a script is defined for the field | ||
| * @param onScriptError the behaviour for when the defined script fails at runtime | ||
| */ | ||
| protected FieldMapper(String simpleName, MappedFieldType mappedFieldType, | ||
| MultiFields multiFields, CopyTo copyTo, | ||
| boolean hasScript, String onScriptError) { | ||
| this(simpleName, mappedFieldType, Collections.emptyMap(), multiFields, copyTo, hasScript, onScriptError); | ||
| } | ||
|
Member
Author
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. if we agree on the new constructors, I think we should try to move all the callers of the ones without the additional parameters to these, otherwise we end up with too many constructors
Contributor
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 like pulling out the common behaviour, but maybe we should try and localise this more? Have an intermediate class called
Member
Author
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 am not sure it's worth the complexity of one additional intermediate base class, especially as more types will support a script.
Contributor
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. Fair enough - we can probably merge the single analyzer and multiple analyzer constructors at least without causing too much noise.
Member
Author
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. agreed, let's do it as a followup? |
||
|
|
||
|
|
||
| /** | ||
| * Create a FieldMapper with a single associated index analyzer | ||
| * @param simpleName the leaf name of the mapper | ||
|
|
@@ -83,7 +101,26 @@ protected FieldMapper(String simpleName, MappedFieldType mappedFieldType, | |
| protected FieldMapper(String simpleName, MappedFieldType mappedFieldType, | ||
| NamedAnalyzer indexAnalyzer, | ||
| MultiFields multiFields, CopyTo copyTo) { | ||
| this(simpleName, mappedFieldType, Collections.singletonMap(mappedFieldType.name(), indexAnalyzer), multiFields, copyTo); | ||
| this(simpleName, mappedFieldType, Collections.singletonMap(mappedFieldType.name(), indexAnalyzer), multiFields, copyTo, | ||
| false, null); | ||
| } | ||
|
|
||
| /** | ||
| * Create a FieldMapper with a single associated index analyzer | ||
| * @param simpleName the leaf name of the mapper | ||
| * @param mappedFieldType the MappedFieldType associated with this mapper | ||
| * @param indexAnalyzer the index-time analyzer to use for this field | ||
| * @param multiFields sub fields of this mapper | ||
| * @param copyTo copyTo fields of this mapper | ||
| * @param hasScript whether a script is defined for the field | ||
| * @param onScriptError the behaviour for when the defined script fails at runtime | ||
| */ | ||
| protected FieldMapper(String simpleName, MappedFieldType mappedFieldType, | ||
| NamedAnalyzer indexAnalyzer, | ||
| MultiFields multiFields, CopyTo copyTo, | ||
| boolean hasScript, String onScriptError) { | ||
| this(simpleName, mappedFieldType, Collections.singletonMap(mappedFieldType.name(), indexAnalyzer), multiFields, copyTo, | ||
| hasScript, onScriptError); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -94,10 +131,13 @@ protected FieldMapper(String simpleName, MappedFieldType mappedFieldType, | |
| * the mapper will add | ||
| * @param multiFields sub fields of this mapper | ||
| * @param copyTo copyTo fields of this mapper | ||
| * @param hasScript whether a script is defined for the field | ||
| * @param onScriptError the behaviour for when the defined script fails at runtime | ||
| */ | ||
| protected FieldMapper(String simpleName, MappedFieldType mappedFieldType, | ||
| Map<String, NamedAnalyzer> indexAnalyzers, | ||
| MultiFields multiFields, CopyTo copyTo) { | ||
| MultiFields multiFields, CopyTo copyTo, | ||
| boolean hasScript, String onScriptError) { | ||
| super(simpleName); | ||
| if (mappedFieldType.name().isEmpty()) { | ||
| throw new IllegalArgumentException("name cannot be empty string"); | ||
|
|
@@ -106,6 +146,8 @@ protected FieldMapper(String simpleName, MappedFieldType mappedFieldType, | |
| this.indexAnalyzers = indexAnalyzers; | ||
| this.multiFields = multiFields; | ||
| this.copyTo = Objects.requireNonNull(copyTo); | ||
| this.hasScript = hasScript; | ||
| this.onScriptError = onScriptError; | ||
| } | ||
|
|
||
| @Override | ||
|
|
@@ -148,6 +190,9 @@ public boolean parsesArrayValue() { | |
| */ | ||
| public void parse(ParseContext context) throws IOException { | ||
| try { | ||
| if (hasScript) { | ||
| throw new IllegalArgumentException("Cannot index data directly into a field with a [script] parameter"); | ||
| } | ||
| parseCreateField(context); | ||
| } catch (Exception e) { | ||
| String valuePreview = ""; | ||
|
|
@@ -172,32 +217,54 @@ public void parse(ParseContext context) throws IOException { | |
| multiFields.parse(this, context); | ||
| } | ||
|
|
||
| /** | ||
| * Parse the field value and populate the fields on {@link ParseContext#doc()}. | ||
| * | ||
| * Implementations of this method should ensure that on failing to parse parser.currentToken() must be the | ||
| * current failing token | ||
| */ | ||
| protected abstract void parseCreateField(ParseContext context) throws IOException; | ||
|
|
||
| /** | ||
| * @return whether this field mapper uses a script to generate its values | ||
| */ | ||
| public boolean hasScript() { | ||
| return false; | ||
| public final boolean hasScript() { | ||
| return hasScript; | ||
| } | ||
|
|
||
| /** | ||
| * Execute the index-time script associated with this field mapper. | ||
| * | ||
| * This method should only be called if {@link #hasScript()} has returned {@code true} | ||
| * @param searchLookup a SearchLookup to be passed the script | ||
| * @param ctx a LeafReaderContext exposing values from an incoming document | ||
| * @param pc the ParseContext over the incoming document | ||
| * @param readerContext a LeafReaderContext exposing values from an incoming document | ||
| * @param doc the id of the document to execute the script against | ||
| * @param parseContext the ParseContext over the incoming document | ||
| */ | ||
| public void executeScript(SearchLookup searchLookup, LeafReaderContext ctx, int doc, ParseContext pc) { | ||
| throw new UnsupportedOperationException("FieldMapper " + name() + " does not have an index-time script"); | ||
| public final void executeScript(SearchLookup searchLookup, LeafReaderContext readerContext, int doc, ParseContext parseContext) { | ||
| try { | ||
| indexScriptValues(searchLookup, readerContext, doc, parseContext); | ||
| } catch (Exception e) { | ||
| if ("ignore".equals(onScriptError)) { | ||
| parseContext.addIgnoredField(name()); | ||
| } else { | ||
| throw new MapperParsingException("Error executing script on field [" + name() + "]", e); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Parse the field value and populate the fields on {@link ParseContext#doc()}. | ||
| * Run the script associated with the field and index the values that it emits | ||
| * | ||
| * Implementations of this method should ensure that on failing to parse parser.currentToken() must be the | ||
| * current failing token | ||
| * This method should only be called if {@link #hasScript()} has returned {@code true} | ||
| * @param searchLookup a SearchLookup to be passed the script | ||
| * @param readerContext a LeafReaderContext exposing values from an incoming document | ||
| * @param doc the id of the document to execute the script against | ||
| * @param parseContext the ParseContext over the incoming document | ||
| */ | ||
| protected abstract void parseCreateField(ParseContext context) throws IOException; | ||
| protected void indexScriptValues(SearchLookup searchLookup, LeafReaderContext readerContext, int doc, ParseContext parseContext) { | ||
| throw new UnsupportedOperationException("FieldMapper " + name() + " does not support [script]"); | ||
| } | ||
|
|
||
| protected final void createFieldNamesField(ParseContext context) { | ||
| assert fieldType().hasDocValues() == false : "_field_names should only be used when doc_values are turned off"; | ||
|
|
@@ -532,8 +599,8 @@ public static final class Parameter<T> implements Supplier<T> { | |
| private MergeValidator<T> mergeValidator; | ||
| private T value; | ||
| private boolean isSet; | ||
| private List<Parameter<?>> requires = new ArrayList<>(); | ||
| private List<Parameter<?>> precludes = new ArrayList<>(); | ||
| private final List<Parameter<?>> requires = new ArrayList<>(); | ||
| private final List<Parameter<?>> precludes = new ArrayList<>(); | ||
|
|
||
| /** | ||
| * Creates a new Parameter | ||
|
|
@@ -875,7 +942,7 @@ public static Parameter<Boolean> docValuesParam(Function<FieldMapper, Boolean> i | |
| * @param initializer retrieves the equivalent parameter from an existing FieldMapper for use in merges | ||
| * @return a script parameter | ||
| */ | ||
| public static FieldMapper.Parameter<Script> scriptParam( | ||
| public static Parameter<Script> scriptParam( | ||
| Function<FieldMapper, Script> initializer | ||
| ) { | ||
| return new FieldMapper.Parameter<>( | ||
|
|
@@ -896,6 +963,20 @@ public static FieldMapper.Parameter<Script> scriptParam( | |
| ).acceptsNull(); | ||
| } | ||
|
|
||
| /** | ||
| * Defines an on_script_error parameter | ||
| * @param initializer retrieves the equivalent parameter from an existing FieldMapper for use in merges | ||
| * @param dependentScriptParam the corresponding required script parameter | ||
| * @return a new on_error_script parameter | ||
| */ | ||
| public static Parameter<String> onScriptErrorParam(Function<FieldMapper, String> initializer, | ||
| Parameter<Script> dependentScriptParam) { | ||
| return Parameter.restrictedStringParam( | ||
| "on_script_error", | ||
| true, | ||
| initializer, | ||
| "reject", "ignore").requiresParameters(dependentScriptParam); | ||
| } | ||
| } | ||
|
|
||
| public static final class Conflicts { | ||
|
|
||
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.
One feature freeze is gone I want to look seriously at moving
mergeto Builder objects, having to carry this stuff around is a real pain.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.
agreed! I am happy to help with that