RuntimeField.Builder should not extend FieldMapper.Builder #73840

romseygeek · 2021-06-07T13:55:18Z

RuntimeField.Builder currently extends FieldMapper.Builder so that it can
share some parsing code and re-use the Parameter infrastructure. However,
this also means that we have to have a number of no-op method implementations,
and in addition this will make it complicated to add a fields parameter within
multi-keyed object field types. This commit removes the class relationship
between these two classes.

elasticmachine · 2021-06-07T13:55:45Z

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

…parser

javanna

left a couple of initial comments, thanks for working on this!

javanna · 2021-06-07T14:12:03Z

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

@@ -58,49 +63,70 @@ default XContentBuilder toXContent(XContentBuilder builder, Params params) throw

    /**
     *  For runtime fields the {@link RuntimeField.Parser} returns directly the {@link MappedFieldType}.


this is also a lie at this point :)

javanna · 2021-06-07T14:15:36Z

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

+                if (propNode == null && parameter.canAcceptNull() == false) {
+                    throw new MapperParsingException("[" + propName + "] on mapper [" + name
+                        + "] of type [" + type + "] must not have a [null] value");
+                }


I wonder if it's worth factoring out this code that is common between the two implementations instead of duplicating it, though it would mean that the base class has to allow to be extended to introduce parsing of common fields etc.

I think the two methods are sufficiently different that it's not worth trying to share stuff here.

are they? I did not realize that

We have not deprecated anything on the runtime section so far, but I do wonder: should we handle deprecations like we do for field mappers?

I guess this makes sense only if we share the code between the two, supporting parameters deprecation while there isn't one deprecated parameter is overkill

++ we can copy the machinery from FieldMapper easily enough if we need to

javanna · 2021-06-07T14:16:37Z

server/src/test/java/org/elasticsearch/index/mapper/AbstractScriptFieldTypeTestCase.java

@@ -115,7 +116,7 @@ public void testMultiFieldsIsNotSupported() throws IOException {
            b.startObject("fields").startObject("test").field("type", "keyword").endObject().endObject();
        });
        MapperParsingException exception = expectThrows(MapperParsingException.class, () -> createMapperService(mapping));
-        assertEquals("Failed to parse mapping: runtime field [field] does not support [fields]", exception.getMessage());
+        assertThat(exception.getMessage(), containsString("unknown parameter [fields] on mapper"));


if we do duplicate the code, I wonder if the message could be tailored to runtime fields, like it used to be. If we decide not to duplicate the code , I guess there is no way around it. In general, I am surprised that we call this "mapper": do users even know what a mapper is?

I've replaced this error message to refer to runtime field instead of mapper

javanna · 2021-06-07T14:18:12Z

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


        @Override
-        public final FieldMapper build(ContentPath context) {
-            throw new UnsupportedOperationException();
+        public final XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {


I've been wondering why the builder is only responsible to print out its parameters. Could we include RuntimeField#toXContent in this so that we don't need to wrap it? If we do so maybe the toXContent within AbstractScriptFieldType could take a RUntimeField.Builder argument instead of ToXCOntent?

let's see if we can do this as a follow-up, not important for merging

javanna · 2021-06-07T14:33:29Z

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

+                            propName,
+                            type
+                        );
+                        iterator.remove();


does this deserve a new test that targets runtime fields specifically?

I've been writing tests for this and I think I've found a bug with the existing impl - we're supposed to issue a deprecation warning if dynamic templates contain mappings with bad parameters, but we actually throw an error because the template validation doesn't propagate its 'within a dynamic template' context. Will open a separate PR for it.

sounds good

javanna · 2021-06-07T14:34:08Z

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

+                if (propNode == null && parameter.canAcceptNull() == false) {
+                    throw new MapperParsingException("[" + propName + "] on mapper [" + name
+                        + "] of type [" + type + "] must not have a [null] value");
+                }


We have not deprecated anything on the runtime section so far, but I do wonder: should we handle deprecations like we do for field mappers?

…parser

romseygeek · 2021-06-16T06:27:54Z

@elasticmachine update branch

…parser

RuntimeField.Builder currently extends FieldMapper.Builder so that it can share some parsing code and re-use the Parameter infrastructure. However, this also means that we have to have a number of no-op method implementations, and in addition this will make it complicated to add a fields parameter within multi-keyed object field types. This commit removes the class relationship between these two classes.

* master: (284 commits) [DOCS] Update central reporting image (elastic#74195) [DOCS] SQL: Document `null` handing for string functions (elastic#74201) Fix Snapshot Docs Listing Query Params in Body Incorrectly (elastic#74196) [DOCS] EQL: Note EQL uses `fields` parameter (elastic#74194) Mute failing MixedClusterClientYamlTestSuiteIT test {p0=indices.split/20_source_mapping/Split index ignores target template mapping} test (elastic#74198) Cleanup Duplicate Constants in Snapshot XContent Params (elastic#74114) [DOC] Add watcher to the threadpool doc (elastic#73935) [Rest Api Compatibility] Validate Query typed api (elastic#74171) Replace deprecated `script.cache.*` settings with `script.context.$constext.cache_*` in documentation. (elastic#74144) Pin Alpine Linux version in Docker builds (elastic#74169) Fix clone API settings docs bug (elastic#74175) [ML] refactor internal datafeed management (elastic#74018) Disable query cache for FunctionScoreQuery and ScriptScoreQuery (elastic#74060) Fork the sending of file chunks during recovery (elastic#74164) RuntimeField.Builder should not extend FieldMapper.Builder (elastic#73840) Run CheckIndex on metadata index before loading (elastic#73239) Deprecate setting version on analyzers (elastic#74073) Add test with null transform id in stats request (elastic#74130) Order imports when reformatting (elastic#74059) Move deprecation code from xpack core to deprecation module. (elastic#74120) ...

RuntimeField.Builder should not extend FieldMapper.Builder

467c26c

romseygeek self-assigned this Jun 7, 2021

romseygeek requested a review from javanna June 7, 2021 13:55

romseygeek added :Search Foundations/Mapping Index mappings, including merging and defining field types >refactoring v7.14.0 v8.0.0 labels Jun 7, 2021

elasticmachine added the Team:Search Meta label for search team label Jun 7, 2021

Merge remote-tracking branch 'origin/master' into runtime/standalone-…

e9e1c85

…parser

javanna reviewed Jun 7, 2021

View reviewed changes

romseygeek added 2 commits June 7, 2021 15:58

cleanup

a0abee4

Merge remote-tracking branch 'origin/master' into runtime/standalone-…

82f5de8

…parser

romseygeek added 2 commits June 16, 2021 08:58

Merge remote-tracking branch 'origin/master' into runtime/standalone-…

8deac31

…parser

error message names

b22257e

romseygeek force-pushed the runtime/standalone-parser branch from 9f765f0 to b22257e Compare June 16, 2021 08:25

javanna approved these changes Jun 16, 2021

View reviewed changes

romseygeek merged commit a18b1cc into elastic:master Jun 16, 2021

romseygeek deleted the runtime/standalone-parser branch June 16, 2021 09:34

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RuntimeField.Builder should not extend FieldMapper.Builder #73840

RuntimeField.Builder should not extend FieldMapper.Builder #73840

romseygeek commented Jun 7, 2021

elasticmachine commented Jun 7, 2021

javanna left a comment

javanna Jun 7, 2021

javanna Jun 7, 2021

romseygeek Jun 7, 2021

javanna Jun 7, 2021

javanna Jun 7, 2021

javanna Jun 16, 2021

romseygeek Jun 16, 2021

javanna Jun 7, 2021

romseygeek Jun 16, 2021

javanna Jun 7, 2021

javanna Jun 15, 2021

javanna Jun 7, 2021

romseygeek Jun 15, 2021

javanna Jun 15, 2021

javanna Jun 7, 2021

romseygeek commented Jun 16, 2021

		@@ -58,49 +63,70 @@ default XContentBuilder toXContent(XContentBuilder builder, Params params) throw

		/**
		* For runtime fields the {@link RuntimeField.Parser} returns directly the {@link MappedFieldType}.

RuntimeField.Builder should not extend FieldMapper.Builder #73840

RuntimeField.Builder should not extend FieldMapper.Builder #73840

Conversation

romseygeek commented Jun 7, 2021

elasticmachine commented Jun 7, 2021

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romseygeek commented Jun 16, 2021