Make ScriptFieldMapper a parameterized mapper by javanna · Pull Request #59391 · elastic/elasticsearch

javanna · 2020-07-13T10:05:14Z

This PR is opened against the runtime fields feature branch.

Relates to #59332

romseygeek

LGTM, I think we should look into making ScriptService available via the parser context which should clear up the SetOnce nastiness.

...ime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptFieldMapper.java

romseygeek · 2020-07-13T10:17:52Z

...ime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptFieldMapper.java

-        private final SetOnce<ScriptService> scriptService = new SetOnce<>();
+        // TODO this is quite ugly and it's static which makes it even worse
+        private static final SetOnce<ScriptService> SCRIPT_SERVICE = new SetOnce<>();



Is there no way of getting it via the ParserContext?

It looks as though we'd have to wire ScriptService into MapperService, but I think that's a reasonable thing to do

I will look into it, thanks for the suggestion.

...ime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptFieldMapper.java

…parameterized

javanna · 2020-07-13T12:23:57Z

...ime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptFieldMapper.java

+    ) {
+        super(simpleName, mappedFieldType, multiFields, copyTo);
+        // TODO is it ok that the object being built needs to read from the object that is building it? Shouldn't Builder#build return
+        // a complete object? Maybe all the parameters need to be passed through instead of the whole builder?


@romseygeek this is for you, does it make sense?

Given that the mapper constructor is protected here, I think it's OK? You can pull out individual parameters if you think it's clearer, I used this pattern on other mappers simply because otherwise you end up with 14 constructor parameters most of which are just boilerplate.

javanna · 2020-07-13T12:24:23Z

...ime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptFieldMapper.java

+            null
+        );
+        // 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?


@romseygeek do you have thoughts on this?

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?

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.

javanna · 2020-07-13T12:25:20Z

...ime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptFieldMapper.java

            }
-            // TODO copy to and multi_fields... not sure what needs to be done.
-            return new ScriptFieldMapper(name, mappedFieldType, multiFieldsBuilder.build(this, context), copyTo);
+            // TODO copy to and multi_fields should not be supported, parametrized field mapper needs to be adapted


@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?

++, I can work on this one.

thanks that would be great!

elasticmachine · 2020-07-13T13:19:33Z

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

javanna · 2020-07-13T13:24:13Z

This is now ready. There are two TODOs left to address which can be addressed later:

rejecting multi fields and copy to on top of script fields
thinking about script updates and their effect on running queries, and what we can do about that

javanna · 2020-07-13T13:42:20Z

...ields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptFieldMapperTests.java

+        assertEquals("Failed to parse mapping: script must be specified for script field [my_field]", exception.getMessage());
+    }
+
+    @AwaitsFix(bugUrl = "Nik: help! :)")


@nik9000 help!

nik9000 · 2020-07-13T13:41:56Z

...ime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptFieldMapper.java

+            null
+        );
+        // 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?


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.

nik9000 · 2020-07-13T13:44:44Z

...ime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptFieldMapper.java

-
-        private final SetOnce<ScriptService> scriptService = new SetOnce<>();
+        @SuppressWarnings("unchecked")
+        static Script parseScript(String name, Mapper.TypeParser.ParserContext parserContext, Object scriptObject) {


I swear I've seen this before somewhere else...

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.

I found it, it was in update by query, I opened #59507 so that we can test and reuse that existing code

nik9000 · 2020-07-13T15:18:20Z

...ime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptFieldMapper.java

-            // TODO these get passed in sometimes and we don't need them
-            node.remove("doc_values");
-            node.remove("index");
+            ScriptFieldMapper.Builder builder = new ScriptFieldMapper.Builder(name, parserContext.queryShardContextSupplier());


I think queryShardContextSupplier exists for the percolator and it is a bit sneaky. It'll do but I'd feel more comfortable if we had the ScriptService directly.

nik9000 · 2020-07-13T15:23:26Z

...ime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptFieldMapper.java

+        super(simpleName, mappedFieldType, multiFields, copyTo);
+        this.runtimeType = runtimeType;
+        this.script = script;
+        this.queryShardContextSupplier = queryShardContextSupplier;


I worry about keeping the queryShardContextSupplier around because it is sneaky. Could getMergeBuilder get the parse context or something like that?

I agree. I personally prefer the hack that we had before (SetOnce in the type parser, set from the plugin class). Otherwise I can get the query shard context from the parser context and pass in only the script service, hopefully it is available.

...ime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptFieldMapper.java

nik9000 · 2020-07-13T15:29:55Z

...ields/src/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptFieldMapperTests.java

+    }
+
+    @AwaitsFix(bugUrl = "Nik: help! :)")
+    public void testDefaultMapping() throws Exception {


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.

the name is from a copy/paste. I wanted to test that the mapper works indeed.

…parameterized

The update by query action parses a script from an object (map or string). We will need to do the same for runtime fields as they are parsed as part of mappings (elastic#59391). This commit moves the existing parsing of a script from an object from RestUpdateByQueryAction to the Script class. It also adds tests and adjusts some error messages that are incorrect. Also, options were not parsed before and they are now. And unsupported fields are no longer silently ignored.

The update by query action parses a script from an object (map or string). We will need to do the same for runtime fields as they are parsed as part of mappings (#59391). This commit moves the existing parsing of a script from an object from RestUpdateByQueryAction to the Script class. It also adds tests and adjusts some error messages that are incorrect. Also, options were not parsed before and they are now. And unsupported fields are no longer silently ignored.

…parameterized

javanna · 2020-07-14T14:28:03Z

I addressed another TODO around the type to be returned and a couple of issues around field caps, as well as added a test for field_caps. This should be ready. I'd like to discuss separately what we should do to clean up how get the reference to ScriptService.

nik9000

LGTM

The update by query action parses a script from an object (map or string). We will need to do the same for runtime fields as they are parsed as part of mappings (#59391). This commit moves the existing parsing of a script from an object from RestUpdateByQueryAction to the Script class. It also adds tests and adjusts some error messages that are incorrect. Also, options were not parsed before and they are now. And unsupported fields trigger now a deprecation warning.

Make ScriptFieldMapper a parameterized mapper

c2ec0a2

javanna added the :Search/Search Search-related issues that do not fall into other categories label Jul 13, 2020

javanna requested review from nik9000 and romseygeek July 13, 2020 10:05

romseygeek approved these changes Jul 13, 2020

View reviewed changes

Merge branch 'feature/runtime_fields' into enhancement/script_mapper_…

c913bbf

…parameterized

javanna commented Jul 13, 2020

View reviewed changes

iter

4458b71

javanna marked this pull request as ready for review July 13, 2020 13:19

elasticmachine added the Team:Search Meta label for search team label Jul 13, 2020

javanna commented Jul 13, 2020

View reviewed changes

nik9000 reviewed Jul 13, 2020

View reviewed changes

javanna added 2 commits July 14, 2020 09:47

Merge branch 'feature/runtime_fields' into enhancement/script_mapper_…

99f1813

…parameterized

iter

993ba66

javanna mentioned this pull request Jul 14, 2020

Consolidate script parsing from object #59507

Merged

javanna added 4 commits July 14, 2020 14:50

Merge branch 'feature/runtime_fields' into enhancement/script_mapper_…

7265381

…parameterized

share parsing code

6b7846d

make test work

67cc50d

iter

0b69696

nik9000 approved these changes Jul 14, 2020

View reviewed changes

javanna merged commit 4eb18b2 into elastic:feature/runtime_fields Jul 14, 2020

javanna mentioned this pull request Jul 15, 2020

Add support for runtime fields #59332

Closed

30 tasks

Conversation

javanna commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

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!

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!

elasticmachine commented Jul 13, 2020

Uh oh!

javanna commented Jul 13, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna commented Jul 14, 2020

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

javanna commented Jul 13, 2020 •

edited

Loading