Share data between DocValuesField and ScriptDocValues #79587
Conversation
jdconrad
added
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
|
||
| import java.io.IOException; | ||
| import java.util.List; | ||
|
|
There was a problem hiding this comment.
Minimal javadoc, please
| } | ||
|
|
||
| class org.elasticsearch.script.field.DelegateDocValuesField @dynamic_type { | ||
| def getValue(def) |
There was a problem hiding this comment.
Why did we need to add this now? I would have figured we needed this when adding unsigned long?
There was a problem hiding this comment.
Delegate field isn't used for unsigned long; it's only used for types that we still provide through only doc. The base Field class doesn't have a getValue or getValues. I added these methods here so a user attempting to access a field type we don't have a new mapped type for, will know they have to use doc.
Edit:
We removed getValue and getValues from the base Field class so that sub classes can return primitives directly with getValue. Dynamic languages can provide this automatically, and non-dynamic languages would require a cast to get the appropriate value type either way, so it didn't make sense to keep them.
| import java.util.List; | ||
|
|
||
| /** | ||
| * A default field to provide {@code ScriptDocValues} for fields |
| @@ -121,7 +121,7 @@ public void testCannotReferToRuntimeFields() throws IOException { | |||
|
|
|||
| Exception e = expectThrows(MapperParsingException.class, () -> mapper.parse(source(b -> {}))); | |||
| assertEquals("Error executing script on field [index-field]", e.getMessage()); | |||
| assertEquals("No field found for [runtime-field] in mapping", e.getCause().getMessage()); | |||
| assertEquals("no field found for [runtime-field] in mapping", e.getCause().getMessage()); | |||
| @@ -589,7 +591,7 @@ public ScoreMode scoreMode() { | |||
|
|
|||
| @Override | |||
| public LeafCollector getLeafCollector(LeafReaderContext context) { | |||
| ScriptDocValues<?> scriptValues = indexFieldData.load(context).getScriptValues(); | |||
| ScriptDocValues<?> scriptValues = indexFieldData.load(context).getScriptField("test").getScriptDocValues(); | |||
There was a problem hiding this comment.
Why here rather than on line 605 and 607?
|
@stu-elastic Thanks for the review comments. I've addressed them, so this should be ready for re-review. |
| @@ -52,7 +51,7 @@ public DocValuesField getScriptField(String fieldName) { | |||
| final MappedFieldType fieldType = fieldTypeLookup.apply(fieldName); | |||
|
|
|||
| if (fieldType == null) { | |||
| throw new IllegalArgumentException("no field found for [" + fieldName + "] in mapping"); | |||
| throw new IllegalArgumentException("No field found for [" + fieldName + "] in mapping"); | |||
There was a problem hiding this comment.
To match the error message that was previously there. I must have changed it when I was moving this from ScriptDocValues to Field.
Edit:
This is why the test had changed.
|
@stu-elastic Thanks again! Will commit as soon as CI passes. |
This change makes it so there is only one path to retrieve values for scripting through the newly introduced fields API. To support backwards compatibility of ScriptDocValues, DocValuesField will return ScriptDocValues for continued doc access where the values are shared, so there is no double loading of field data. For now, for unsupported DocValuesFields we have a DelegateDocValuesField that returns the ScriptDocValues for long, double, String, etc.
This change makes it so there is only one path to retrieve values for scripting through the newly introduced fields API. To support backwards compatibility of
ScriptDocValues,DocValuesFieldwill returnScriptDocValuesfor continueddocaccess where the values are shared, so there is no double loading of field data. For now, for unsupportedDocValuesFields we have aDelegateDocValuesFieldthat returns theScriptDocValuesfor long, double, String, etc.