Add support for script to boolean field mapper #71454

javanna · 2021-04-08T08:57:11Z

Relates to #68984

javanna · 2021-04-08T08:58:36Z

server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java

@@ -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);


I wonder if instead of null we should have some impl of ScriptCompiler that throws UnsupportedOperationException when used.

++, either here or in a follow up - it will be useful on the other field mappers as well

javanna · 2021-04-08T08:59:17Z

server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java

+                          MultiFields multiFields, CopyTo copyTo,
+                          boolean hasScript, String onScriptError) {
+        this(simpleName, mappedFieldType, Collections.emptyMap(), multiFields, copyTo, hasScript, onScriptError);
+    }


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

I like pulling out the common behaviour, but maybe we should try and localise this more? Have an intermediate class called ScriptableFieldMapper or something like that?

I am not sure it's worth the complexity of one additional intermediate base class, especially as more types will support a script.

Fair enough - we can probably merge the single analyzer and multiple analyzer constructors at least without causing too much noise.

agreed, let's do it as a followup?

romseygeek

I left a couple of suggestions, I'm guessing tests will come in soon now that #71322 has been merged?

romseygeek · 2021-04-08T12:36:50Z

server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java

+                          MultiFields multiFields, CopyTo copyTo,
+                          boolean hasScript, String onScriptError) {
+        this(simpleName, mappedFieldType, Collections.emptyMap(), multiFields, copyTo, hasScript, onScriptError);
+    }


I like pulling out the common behaviour, but maybe we should try and localise this more? Have an intermediate class called ScriptableFieldMapper or something like that?

romseygeek · 2021-04-08T12:40:51Z

server/src/main/java/org/elasticsearch/script/BooleanFieldScript.java

+        runForDoc(docId);
+        int count = trues + falses;
+        for (int i = 0; i < count; i++) {
+            consumer.accept(i >= falses);


I think we need to emit true trues times and then false falses times here? Otherwise we're collapsing things down to a single value where there may be multiple values in the source.

I think that is what I am doing: count is trues + falses. I emit false until there are falses left, and then trues. I may be missing something though as I don't follow the collapsing concern you mentioned :)

duh yes, I am clearly not reading properly today. Maybe add a comment?

Maybe it's because the code is cryptic. this is some simplification that IntelliJ suggested, I should go back to something more readable although longer.

elasticmachine · 2021-04-08T14:37:16Z

Pinging @elastic/es-search (Team:Search)

javanna · 2021-04-08T15:19:11Z

I pushed an update including docs and yaml tests

romseygeek

LGTM!

romseygeek · 2021-04-08T16:36:39Z

docs/reference/mapping/types/boolean.asciidoc

+    the document will be rejected with an error.
+    Scripts are in the same format as their
+    <<runtime-mapping-fields,runtime equivalent>>. Scripts can only be
+    configured on `long` and `double` field types.


Don't need this last sentence for booleans :)

romseygeek · 2021-04-08T16:38:02Z

server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java

@@ -208,14 +242,21 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower
    private final boolean indexed;
    private final boolean hasDocValues;
    private final boolean stored;
+    private final Script script;
+    private final FieldValues<Boolean> scriptValues;
+    private final ScriptCompiler scriptCompiler;


One feature freeze is gone I want to look seriously at moving merge to Builder objects, having to carry this stuff around is a real pain.

agreed! I am happy to help with that

romseygeek · 2021-04-08T16:38:44Z

server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java

@@ -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);


++, either here or in a follow up - it will be useful on the other field mappers as well

romseygeek · 2021-04-08T16:40:13Z

server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java

+                          MultiFields multiFields, CopyTo copyTo,
+                          boolean hasScript, String onScriptError) {
+        this(simpleName, mappedFieldType, Collections.emptyMap(), multiFields, copyTo, hasScript, onScriptError);
+    }


Fair enough - we can probably merge the single analyzer and multiple analyzer constructors at least without causing too much noise.

romseygeek · 2021-04-08T16:44:38Z

test/framework/src/main/java/org/elasticsearch/index/mapper/MapperScriptTestCase.java

        if (script.getIdOrCode().equals("serializer_test")) {
-            return (T) serializableScript();
+            return (T)serializableScript();


nit: I definitely prefer a space here...

this was not meant to happen :)

Relates to elastic#68984

Relates to #68984

javanna added 3 commits April 7, 2021 21:46

Add support for script to boolean field mapper

f6346d5

Relates to elastic#68984

iter

ec26992

iter

6df4380

javanna added >enhancement :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.13.0 labels Apr 8, 2021

javanna requested a review from romseygeek April 8, 2021 08:57

javanna commented Apr 8, 2021

View reviewed changes

javanna added 2 commits April 8, 2021 12:59

merge

2c42e95

Merge branch 'master' into enhancement/boolean_mapper_script

e97452f

romseygeek reviewed Apr 8, 2021

View reviewed changes

tests

55511d5

javanna marked this pull request as ready for review April 8, 2021 14:37

elasticmachine added the Team:Search Meta label for search team label Apr 8, 2021

javanna added 3 commits April 8, 2021 16:45

clarify runForDoc

34d4353

docs

fd81169

yaml tests

e759f87

javanna mentioned this pull request Apr 8, 2021

Allow fields calculated by scripting at index time #68984

Closed

9 tasks

romseygeek approved these changes Apr 8, 2021

View reviewed changes

javanna added 5 commits April 8, 2021 22:18

iter

ab103a7

spotless

38d2203

iter

30ef85e

iter

7fd4e4c

iter

142623f

javanna merged commit 1469e18 into elastic:master Apr 12, 2021

javanna added a commit to javanna/elasticsearch that referenced this pull request Apr 12, 2021

Add support for script parameter to boolean field mapper (elastic#71454)

44372b7

Relates to elastic#68984

javanna added a commit that referenced this pull request Apr 12, 2021

Add support for script parameter to boolean field mapper (#71454)

29a875f

Relates to #68984

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Add support for script to boolean field mapper #71454

Add support for script to boolean field mapper #71454

Uh oh!

Conversation

javanna commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Apr 8, 2021

Uh oh!

javanna commented Apr 8, 2021

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

javanna commented Apr 8, 2021 •

edited

Loading