Add long flavored script field #59721

nik9000 · 2020-07-16T18:42:16Z

This adds the long runtime type to script fields and implements doc
values, field data, the term and exists query.

Relates to #59332

This adds the `long` runtime type to `script` fields and implements doc values, field data, the `term` and `exists` query.

elasticmachine · 2020-07-16T18:42:27Z

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

javanna · 2020-07-17T12:45:34Z

run elasticsearch-ci/1

javanna

I left some comments, I like it ;)

javanna · 2020-07-17T13:12:40Z

...untime-fields/src/main/java/org/elasticsearch/xpack/runtimefields/LongScriptFieldScript.java

+        return count;
+    }
+
+    protected final void add(long v) {


does it need to be protected? Also maybe rename to something like collectValue ? I find it weird to call add against the script itself

does it need to be protected?

I thought so, but I'll take a look.

Also maybe rename to something like collectValue

👍

javanna · 2020-07-17T13:14:28Z

...ields/src/main/java/org/elasticsearch/xpack/runtimefields/fielddata/ScriptLongDocValues.java

+import java.io.IOException;
+import java.util.Arrays;
+
+public final class ScriptLongDocValues extends AbstractSortedNumericDocValues {


you can use SortingNumericDocValues?

It'd force me to copy the longs from one array to the other rather than just sort. We already have to copy string because of BytesRef vs String stuff. But here I don't have to copy unless I want to.

I see, makes sense then

...ields/src/main/java/org/elasticsearch/xpack/runtimefields/fielddata/ScriptLongFieldData.java

javanna · 2020-07-17T13:16:15Z

...ields/src/main/java/org/elasticsearch/xpack/runtimefields/fielddata/ScriptLongFieldData.java

+
+    @Override
+    public void setSearchLookup(SearchLookup searchLookup) {
+        // TODO wire the params from the mappings definition, we don't parse them yet


if you carry the script around you can remove this TODO :)

javanna · 2020-07-17T13:17:29Z

...ields/src/main/java/org/elasticsearch/xpack/runtimefields/fielddata/ScriptLongFieldData.java

+        try {
+            return loadDirect(context);
+        } catch (Exception e) {
+            if (e instanceof ElasticsearchException) {


another change that I lost somehow, but this should rather be ExceptionsHelper#convertToElastic . Can you make the same change in the binary one too?

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

.../src/main/java/org/elasticsearch/xpack/runtimefields/query/AbstractLongScriptFieldQuery.java

+                    @Override
+                    public float matchCost() {
+                        // TODO we don't have a good way of estimating the complexity of the script so we just go with 9000
+                        return 9000f;


Shall we make this a shared constant somewhere, and remove the TODO as that is what we will keep for the time being?

javanna · 2020-07-17T13:24:55Z

...ds/src/main/java/org/elasticsearch/xpack/runtimefields/query/LongScriptFieldExistsQuery.java

+    @Override
+    public final String toString(String field) {
+        if (fieldName().contentEquals(field)) {
+            return "ScriptFieldExists";


I would be fine with even printing the getSimpleName

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

...c/test/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptLongMappedFieldTypeTests.java

javanna · 2020-07-17T13:29:37Z

x-pack/plugin/src/test/resources/rest-api-spec/test/runtime_fields/10_keyword.yml

@@ -14,7 +14,7 @@ setup:
              temperature:
                type: long
              voltage:
-                type: float
+                type: double


Sneaky rounding error in the multiplication in the test script had me waste an hour trying to find out why 5.1 * 10 == 52. Stupid floating point.

javanna · 2020-07-17T18:51:32Z

...time-fields/src/main/java/org/elasticsearch/xpack/runtimefields/StringScriptFieldScript.java

@@ -35,7 +35,7 @@
        StringScriptFieldScript newInstance(LeafReaderContext ctx) throws IOException;
    }

-    protected final List<String> results = new ArrayList<>();
+    private final List<String> results = new ArrayList<>();


thanks! that was my bad after all :D

javanna · 2020-07-17T18:53:13Z

...rc/main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java

+
+    void mapperXContentBody(XContentBuilder builder, Params params) throws IOException {
+        builder.field("runtime_type", runtimeType());
+        builder.field("script", script.getIdOrCode()); // TODO For some reason this doesn't allow us to do the full xcontent of the script.


I no longer remember what this TODO is about. what can we do to address it? and what do we need this method for, again?

It spits out the script in the xcontent of the mapper. I think we might want to merge it into the FieldMapper's xcontent rendering. I can have a look in a follow up.

That TODO is basically complaining that I can render the whole script - it won't parse properly if I do. I don't remember why. I'll dig.

I opened #59813 for the TODO.

javanna · 2020-07-17T18:54:11Z

...src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptKeywordMappedFieldType.java


-public final class RuntimeKeywordMappedFieldType extends MappedFieldType {
+public final class ScriptKeywordMappedFieldType extends AbstractScriptMappedFieldType {


javanna · 2020-07-17T18:58:21Z

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

+        FieldMapper mapper = (FieldMapper) mapperService.documentMapper().mappers().getMapper("field");
+        assertThat(mapper, instanceOf(ScriptFieldMapper.class));
+        assertEquals(Strings.toString(mapping("long")), Strings.toString(mapperService.documentMapper()));
+    }


maybe adding one of these methods could even be automatic once a new supported field type is added to the map? not sure that it's important now though

Its a good idea. We could loop through the list but I kind of like having a method for each one.

javanna

left some minors here and there but LGTM

Add long flavored script field

5ff1e75

This adds the `long` runtime type to `script` fields and implements doc values, field data, the `term` and `exists` query.

nik9000 requested a review from javanna July 16, 2020 18:42

nik9000 force-pushed the script_field_long_1 branch from 2896a01 to 5ff1e75 Compare July 16, 2020 18:42

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

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

javanna mentioned this pull request Jul 16, 2020

Add support for runtime fields #59332

Closed

30 tasks

javanna reviewed Jul 17, 2020

View reviewed changes

nik9000 mentioned this pull request Jul 17, 2020

Scripted keyword field type: update family type and test field caps output #59672

Merged

nik9000 added 3 commits July 17, 2020 12:57

Merge branch 'feature/runtime_fields' into script_field_long_1

f5ea47a

iter

66f11a0

checkstyle

1763c20

nik9000 requested a review from javanna July 17, 2020 17:36

javanna reviewed Jul 17, 2020

View reviewed changes

javanna approved these changes Jul 17, 2020

View reviewed changes

nik9000 merged commit c8b5af9 into elastic:feature/runtime_fields Jul 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add long flavored script field #59721

Add long flavored script field #59721

nik9000 commented Jul 16, 2020

elasticmachine commented Jul 16, 2020

javanna commented Jul 17, 2020

javanna left a comment

javanna Jul 17, 2020

nik9000 Jul 17, 2020

javanna Jul 17, 2020

nik9000 Jul 17, 2020

javanna Jul 17, 2020

javanna Jul 17, 2020

nik9000 Jul 17, 2020

javanna Jul 17, 2020

nik9000 Jul 17, 2020

javanna Jul 17, 2020

nik9000 Jul 17, 2020

javanna Jul 17, 2020

nik9000 Jul 17, 2020

javanna Jul 17, 2020

nik9000 Jul 17, 2020

javanna Jul 17, 2020

javanna Jul 17, 2020

nik9000 Jul 17, 2020

nik9000 Jul 17, 2020

javanna Jul 17, 2020

javanna Jul 17, 2020

nik9000 Jul 17, 2020

javanna left a comment


		public final class RuntimeKeywordMappedFieldType extends MappedFieldType {
		public final class ScriptKeywordMappedFieldType extends AbstractScriptMappedFieldType {

Add long flavored script field #59721

Add long flavored script field #59721

Conversation

nik9000 commented Jul 16, 2020

elasticmachine commented Jul 16, 2020

javanna commented Jul 17, 2020

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

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

javanna left a comment

Choose a reason for hiding this comment