From 541a4fd72ba2fbf62e6cf094d0ebb997b6d067e4 Mon Sep 17 00:00:00 2001 From: Jack Conradson Date: Tue, 19 Oct 2021 15:25:05 -0700 Subject: [PATCH] Remove getValue and getValues from Field (#79516) With duck-typing in Painless and potentially non-dynamic languages requiring a cast under the current design, this change removes unnecessary methods from the Field base class. The removal of getValue and getValues allows sub classes to return primitives from these methods and opens up potential design space for performance improvements. Note this removes getLong from the unsigned long field; however, this is currently undocumented and unused behavior. --- .../org.elasticsearch.script.fields.txt | 3 --- .../index/fielddata/LeafFieldData.java | 2 +- .../elasticsearch/script/DocBasedScript.java | 6 +++--- .../org/elasticsearch/script/DocReader.java | 4 ++-- .../script/DocValuesDocReader.java | 6 +++--- .../script/field/DocValuesField.java | 2 +- .../script/field/EmptyField.java | 20 +------------------ .../org/elasticsearch/script/field/Field.java | 13 +----------- .../search/lookup/LeafDocLookup.java | 10 +++++----- .../plugin/mapper-unsigned-long/build.gradle | 8 +++++++- .../UnsignedLongDocValuesField.java | 20 +++---------------- .../xpack/unsignedlong/UnsignedLongField.java | 9 ++++++--- .../UnsignedLongLeafFieldData.java | 2 +- .../org.elasticsearch.xpack.unsignedlong.txt | 5 +++-- .../rest-api-spec/test/50_script_values.yml | 14 ------------- 15 files changed, 37 insertions(+), 87 deletions(-) diff --git a/modules/lang-painless/src/main/resources/org/elasticsearch/painless/org.elasticsearch.script.fields.txt b/modules/lang-painless/src/main/resources/org/elasticsearch/painless/org.elasticsearch.script.fields.txt index 61986fab9423c..af330de477216 100644 --- a/modules/lang-painless/src/main/resources/org/elasticsearch/painless/org.elasticsearch.script.fields.txt +++ b/modules/lang-painless/src/main/resources/org/elasticsearch/painless/org.elasticsearch.script.fields.txt @@ -13,9 +13,6 @@ class org.elasticsearch.script.field.Field @dynamic_type { String getName() boolean isEmpty() int size() - List getValues() - def getValue(def) - def getValue(int, def) } class org.elasticsearch.script.DocBasedScript { diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/LeafFieldData.java b/server/src/main/java/org/elasticsearch/index/fielddata/LeafFieldData.java index 69cf081da0d04..6e3b0f8135918 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/LeafFieldData.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/LeafFieldData.java @@ -28,7 +28,7 @@ public interface LeafFieldData extends Accountable, Releasable { /** * Returns an {@code Field} for use in accessing field values in scripting. */ - default DocValuesField getScriptField(String name) { + default DocValuesField getScriptField(String name) { throw new UnsupportedOperationException(); } diff --git a/server/src/main/java/org/elasticsearch/script/DocBasedScript.java b/server/src/main/java/org/elasticsearch/script/DocBasedScript.java index 8ef8104652aed..6a05eb0b70ffb 100644 --- a/server/src/main/java/org/elasticsearch/script/DocBasedScript.java +++ b/server/src/main/java/org/elasticsearch/script/DocBasedScript.java @@ -23,14 +23,14 @@ public DocBasedScript(DocReader docReader) { this.docReader = docReader; } - public Field field(String fieldName) { + public Field field(String fieldName) { if (docReader == null) { - return new EmptyField<>(fieldName); + return new EmptyField(fieldName); } return docReader.field(fieldName); } - public Stream> fields(String fieldGlob) { + public Stream fields(String fieldGlob) { if (docReader == null) { return Stream.empty(); } diff --git a/server/src/main/java/org/elasticsearch/script/DocReader.java b/server/src/main/java/org/elasticsearch/script/DocReader.java index 19f6f157f542b..17ea73c576026 100644 --- a/server/src/main/java/org/elasticsearch/script/DocReader.java +++ b/server/src/main/java/org/elasticsearch/script/DocReader.java @@ -22,10 +22,10 @@ */ public interface DocReader { /** New-style field access */ - Field field(String fieldName); + Field field(String fieldName); /** New-style field iterator */ - Stream> fields(String fieldGlob); + Stream fields(String fieldGlob); /** Set the underlying docId */ void setDocument(int docID); diff --git a/server/src/main/java/org/elasticsearch/script/DocValuesDocReader.java b/server/src/main/java/org/elasticsearch/script/DocValuesDocReader.java index b295736fdb8d4..0462292f67ddd 100644 --- a/server/src/main/java/org/elasticsearch/script/DocValuesDocReader.java +++ b/server/src/main/java/org/elasticsearch/script/DocValuesDocReader.java @@ -39,11 +39,11 @@ public DocValuesDocReader(SearchLookup searchLookup, LeafReaderContext leafConte } @Override - public Field field(String fieldName) { + public Field field(String fieldName) { LeafDocLookup leafDocLookup = leafSearchLookup.doc(); if (leafDocLookup.containsKey(fieldName) == false) { - return new EmptyField<>(fieldName); + return new EmptyField(fieldName); } return leafDocLookup.getScriptField(fieldName); @@ -51,7 +51,7 @@ public Field field(String fieldName) { @Override - public Stream> fields(String fieldGlob) { + public Stream fields(String fieldGlob) { throw new UnsupportedOperationException("not implemented"); } diff --git a/server/src/main/java/org/elasticsearch/script/field/DocValuesField.java b/server/src/main/java/org/elasticsearch/script/field/DocValuesField.java index 9a48c3aa8f871..a412f4dc2a08a 100644 --- a/server/src/main/java/org/elasticsearch/script/field/DocValuesField.java +++ b/server/src/main/java/org/elasticsearch/script/field/DocValuesField.java @@ -10,7 +10,7 @@ import java.io.IOException; -public interface DocValuesField extends Field { +public interface DocValuesField extends Field { /** Set the current document ID. */ void setNextDocId(int docId) throws IOException; diff --git a/server/src/main/java/org/elasticsearch/script/field/EmptyField.java b/server/src/main/java/org/elasticsearch/script/field/EmptyField.java index 968a82ebc9660..c2b7252af5bfd 100644 --- a/server/src/main/java/org/elasticsearch/script/field/EmptyField.java +++ b/server/src/main/java/org/elasticsearch/script/field/EmptyField.java @@ -8,13 +8,10 @@ package org.elasticsearch.script.field; -import java.util.Collections; -import java.util.List; - /** * Script field with no mapping, always returns {@code defaultValue}. */ -public class EmptyField implements Field { +public class EmptyField implements Field { private final String name; @@ -36,19 +33,4 @@ public boolean isEmpty() { public int size() { return 0; } - - @Override - public List getValues() { - return Collections.emptyList(); - } - - @Override - public T getValue(T defaultValue) { - return defaultValue; - } - - @Override - public T getValue(int index, T defaultValue) { - return defaultValue; - } } diff --git a/server/src/main/java/org/elasticsearch/script/field/Field.java b/server/src/main/java/org/elasticsearch/script/field/Field.java index 3d927afd3dd32..e13732fa46785 100644 --- a/server/src/main/java/org/elasticsearch/script/field/Field.java +++ b/server/src/main/java/org/elasticsearch/script/field/Field.java @@ -9,10 +9,8 @@ package org.elasticsearch.script.field; -import java.util.List; - /** A field in a document accessible via scripting. */ -public interface Field { +public interface Field { /** Returns the name of this field. */ String getName(); @@ -22,13 +20,4 @@ public interface Field { /** Returns the number of values this field has. */ int size(); - - /** Get all values of a multivalued field. If {@code isEmpty()} this returns an empty list. */ - List getValues(); - - /** Get the first value of a field, if {@code isEmpty()} return defaultValue instead */ - T getValue(T defaultValue); - - /** Get the value of a field as the specified index. */ - T getValue(int index, T defaultValue); } diff --git a/server/src/main/java/org/elasticsearch/search/lookup/LeafDocLookup.java b/server/src/main/java/org/elasticsearch/search/lookup/LeafDocLookup.java index 9352e91341c9b..d2198b237548d 100644 --- a/server/src/main/java/org/elasticsearch/search/lookup/LeafDocLookup.java +++ b/server/src/main/java/org/elasticsearch/search/lookup/LeafDocLookup.java @@ -25,7 +25,7 @@ public class LeafDocLookup implements Map> { - private final Map> localCacheScriptFieldData = new HashMap<>(4); + private final Map localCacheScriptFieldData = new HashMap<>(4); private final Map> localCacheFieldData = new HashMap<>(4); private final Function fieldTypeLookup; private final Function> fieldDataLookup; @@ -45,8 +45,8 @@ public void setDocument(int docId) { this.docId = docId; } - public DocValuesField getScriptField(String fieldName) { - DocValuesField field = localCacheScriptFieldData.get(fieldName); + public DocValuesField getScriptField(String fieldName) { + DocValuesField field = localCacheScriptFieldData.get(fieldName); if (field == null) { final MappedFieldType fieldType = fieldTypeLookup.apply(fieldName); @@ -57,9 +57,9 @@ public DocValuesField getScriptField(String fieldName) { // Load the field data on behalf of the script. Otherwise, it would require // additional permissions to deal with pagedbytes/ramusagestimator/etc. - field = AccessController.doPrivileged(new PrivilegedAction>() { + field = AccessController.doPrivileged(new PrivilegedAction() { @Override - public DocValuesField run() { + public DocValuesField run() { return fieldDataLookup.apply(fieldType).load(reader).getScriptField(fieldName); } }); diff --git a/x-pack/plugin/mapper-unsigned-long/build.gradle b/x-pack/plugin/mapper-unsigned-long/build.gradle index 4eac800de79d4..00cf286797afa 100644 --- a/x-pack/plugin/mapper-unsigned-long/build.gradle +++ b/x-pack/plugin/mapper-unsigned-long/build.gradle @@ -30,4 +30,10 @@ restResources { restApi { include '_common', 'bulk', 'indices', 'index', 'search', 'xpack' } -} \ No newline at end of file +} + +tasks.named("yamlRestTestV7CompatTest").configure { + systemProperty 'tests.rest.blacklist', [ + '50_script_values/Scripted fields values return Long' + ].join(',') +} diff --git a/x-pack/plugin/mapper-unsigned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongDocValuesField.java b/x-pack/plugin/mapper-unsigned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongDocValuesField.java index 9739b63e9da7d..a5407719d5f93 100644 --- a/x-pack/plugin/mapper-unsigned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongDocValuesField.java +++ b/x-pack/plugin/mapper-unsigned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongDocValuesField.java @@ -20,7 +20,7 @@ import static org.elasticsearch.search.DocValueFormat.MASK_2_63; import static org.elasticsearch.xpack.unsignedlong.UnsignedLongFieldMapper.BIGINTEGER_2_64_MINUS_ONE; -public class UnsignedLongDocValuesField implements UnsignedLongField, DocValuesField { +public class UnsignedLongDocValuesField implements UnsignedLongField, DocValuesField { private final SortedNumericDocValues input; private long[] values = new long[0]; @@ -89,26 +89,12 @@ public List getValues() { } @Override - public Long getValue(Long defaultValue) { + public long getValue(long defaultValue) { return getValue(0, defaultValue); } @Override - public Long getValue(int index, Long defaultValue) { - if (isEmpty() || index < 0 || index >= count) { - return defaultValue; - } - - return toFormatted(index); - } - - @Override - public long getLong(long defaultValue) { - return getLong(0, defaultValue); - } - - @Override - public long getLong(int index, long defaultValue) { + public long getValue(int index, long defaultValue) { if (isEmpty() || index < 0 || index >= count) { return defaultValue; } diff --git a/x-pack/plugin/mapper-unsigned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongField.java b/x-pack/plugin/mapper-unsigned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongField.java index 96b8c730eed69..acf69cd7ae3bd 100644 --- a/x-pack/plugin/mapper-unsigned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongField.java +++ b/x-pack/plugin/mapper-unsigned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongField.java @@ -12,13 +12,16 @@ import java.math.BigInteger; import java.util.List; -public interface UnsignedLongField extends Field { +public interface UnsignedLongField extends Field { /** Returns the 0th index value as an {@code long} if it exists, otherwise {@code defaultValue}. */ - long getLong(long defaultValue); + long getValue(long defaultValue); /** Returns the value at {@code index} as an {@code long} if it exists, otherwise {@code defaultValue}. */ - long getLong(int index, long defaultValue); + long getValue(int index, long defaultValue); + + /** Return all the values as a {@code List}. */ + List getValues(); /** Returns the 0th index value as a {@code BigInteger} if it exists, otherwise {@code defaultValue}. */ BigInteger getBigInteger(BigInteger defaultValue); diff --git a/x-pack/plugin/mapper-unsigned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongLeafFieldData.java b/x-pack/plugin/mapper-unsigned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongLeafFieldData.java index 30575818d169a..63e4e6d8dc142 100644 --- a/x-pack/plugin/mapper-unsigned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongLeafFieldData.java +++ b/x-pack/plugin/mapper-unsigned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongLeafFieldData.java @@ -79,7 +79,7 @@ public ScriptDocValues getScriptValues() { } @Override - public DocValuesField getScriptField(String name) { + public DocValuesField getScriptField(String name) { return new UnsignedLongDocValuesField(getLongValues(), name); } diff --git a/x-pack/plugin/mapper-unsigned-long/src/main/resources/org/elasticsearch/xpack/unsignedlong/org.elasticsearch.xpack.unsignedlong.txt b/x-pack/plugin/mapper-unsigned-long/src/main/resources/org/elasticsearch/xpack/unsignedlong/org.elasticsearch.xpack.unsignedlong.txt index bebec2e2a47da..574a535714d3d 100644 --- a/x-pack/plugin/mapper-unsigned-long/src/main/resources/org/elasticsearch/xpack/unsignedlong/org.elasticsearch.xpack.unsignedlong.txt +++ b/x-pack/plugin/mapper-unsigned-long/src/main/resources/org/elasticsearch/xpack/unsignedlong/org.elasticsearch.xpack.unsignedlong.txt @@ -12,8 +12,9 @@ class org.elasticsearch.xpack.unsignedlong.UnsignedLongScriptDocValues { } class org.elasticsearch.xpack.unsignedlong.UnsignedLongField @dynamic_type { - long getLong(long) - long getLong(int, long) + long getValue(long) + long getValue(int, long) + List getValues() BigInteger getBigInteger(BigInteger) BigInteger getBigInteger(int, BigInteger) List getBigIntegers() diff --git a/x-pack/plugin/mapper-unsigned-long/src/yamlRestTest/resources/rest-api-spec/test/50_script_values.yml b/x-pack/plugin/mapper-unsigned-long/src/yamlRestTest/resources/rest-api-spec/test/50_script_values.yml index c43f22a193f38..7a4be63265679 100644 --- a/x-pack/plugin/mapper-unsigned-long/src/yamlRestTest/resources/rest-api-spec/test/50_script_values.yml +++ b/x-pack/plugin/mapper-unsigned-long/src/yamlRestTest/resources/rest-api-spec/test/50_script_values.yml @@ -46,21 +46,7 @@ setup: - match: { hits.hits.2.fields.scripted_ul.0: -9223372036854775808 } - match: { hits.hits.3.fields.scripted_ul.0: 9223372036854775807 } - match: { hits.hits.4.fields.scripted_ul.0: 0 } - - do: - search: - index: test1 - body: - sort: [ { ul: desc } ] - script_fields: - scripted_ul: - script: - source: "field('ul').getLong(1000L)" - - match: { hits.hits.0.fields.scripted_ul.0: -1 } - - match: { hits.hits.1.fields.scripted_ul.0: -3 } - - match: { hits.hits.2.fields.scripted_ul.0: -9223372036854775808 } - - match: { hits.hits.3.fields.scripted_ul.0: 9223372036854775807 } - - match: { hits.hits.4.fields.scripted_ul.0: 0 } - do: search: index: test1