Separate AbstractScriptFieldType from RuntimeField implementations #74768
Conversation
romseygeek
added
:Search Foundations/Mapping
|
Pinging @elastic/es-search (Team:Search) |
server/src/main/java/org/elasticsearch/index/mapper/BooleanScriptFieldType.java
Show resolved
Hide resolved
| import java.util.Collection; | ||
| import java.util.Collections; | ||
|
|
||
| public abstract class ScriptRuntimeField implements RuntimeField { |
There was a problem hiding this comment.
I am a bit on the fence on the naming, mostly because the object field is also going to hold a script but will not extend this class. Maybe this could be named LeafRuntimeField or something along those lines?
There was a problem hiding this comment.
++ to LeafRuntimeField, the main use of this base class is to simplify returning a single-valued collection from asMappedFieldTypes() and 'Leaf' reinforces that.
| import java.util.Collection; | ||
| import java.util.Collections; | ||
|
|
||
| public abstract class ScriptRuntimeField implements RuntimeField { |
There was a problem hiding this comment.
can you also add javadocs to explain what this class is intended for?
|
|
||
| public abstract class ScriptRuntimeField implements RuntimeField { | ||
|
|
||
| protected final String name; |
There was a problem hiding this comment.
this one corresponds to the simple field name in ObjectMapper right? It is only the name of the leaf field, although it can still contain dots? I wonder if we should clearly document this.
There was a problem hiding this comment.
We don't have object fields yet so there isn't a distinction here at the moment. Let's document it when we know precisely how the two different names will work?
| @@ -288,6 +288,6 @@ public void execute() { | |||
| } | |||
|
|
|||
| private static IpScriptFieldType build(Script script) { | |||
| return new IpScriptFieldType("test", factory(script), script, emptyMap(), (builder, params) -> builder); | |||
| return new IpScriptFieldType("test", factory(script), script, emptyMap()); | |||
| @@ -217,16 +217,14 @@ public void testRuntimeSectionMerge() throws IOException { | |||
| merge(mapperService, mapping); | |||
| DoubleScriptFieldType field = (DoubleScriptFieldType)mapperService.fieldType("field"); | |||
| assertEquals(NumberFieldMapper.NumberType.DOUBLE.typeName(), field.typeName()); | |||
| LongScriptFieldType field2Updated = (LongScriptFieldType)mapperService.fieldType("field2"); | |||
| assertSame(field2, field2Updated); | |||
| assertThat(mapperService.fieldType("field2"), instanceOf(LongScriptFieldType.class)); | |||
There was a problem hiding this comment.
so, now the runtime field is the same instance but not the mapped field type, or?
There was a problem hiding this comment.
which makes me wonder: do we need to return a new instance each time asMappedFieldType is called? Probably not?
There was a problem hiding this comment.
We probably could create fixed MappedFieldTypes at construction time and always return the same object, but this is only called when Mappings are re-parsed so I'm not sure it really makes a lot of difference.
There was a problem hiding this comment.
it certainly complicates the test no? What do we do for field mappers? Don't we always expose the same field type, that is created at construction time?
There was a problem hiding this comment.
Have pushed a change to built the field types up front.
romseygeek
added
the
auto-backport
💔 Backport failed
To backport manually run: |
…74768) AbstractScriptFieldType extends MappedFieldType and also implements RuntimeField. This means that the name() of a runtime field has to be the same as the name() of its corresponding mapped field type. However, for object fields we will want the nested child fields to have a specific short name, while their MappedFieldType will need to have a name that includes the parent. To make this distinction possible, this commit changes AbstractScriptFieldType subclasses to return their own implementations of RuntimeField from their type parsers instead of implementing the interface directly.
AbstractScriptFieldType extends MappedFieldType and also implements
RuntimeField. This means that the name() of a runtime field has to be the
same as the name() of its corresponding mapped field type. However, for
object fields we will want the nested child fields to have a specific short name,
while their MappedFieldType will need to have a name that includes the parent.
To make this distinction possible, this commit changes AbstractScriptFieldType
subclasses to return their own implementations of RuntimeField from their
type parsers instead of implementing the interface directly.