From b4c84d9a203e264ed3f713d0e4f75d1d85bebef6 Mon Sep 17 00:00:00 2001 From: Mayya Sharipova Date: Mon, 9 Jul 2018 11:59:49 -0400 Subject: [PATCH] Handle missing values in painless (#30975) Throw an exception for `doc['field'].value` if this document is missing a value for the `field`. For 7.0: This is the default behaviour from 7.0 For 6.x: To enable this behavior from 6.x, a user can set a jvm.option: `-Des.script.exception_for_missing_value=true` on a node. If a user does not enable this behavior, a deprecation warning is logged on start up. Closes #29286 --- .../elasticsearch/gradle/BuildPlugin.groovy | 1 + .../painless-getting-started.asciidoc | 24 +++ modules/lang-painless/build.gradle | 2 +- .../test/painless/20_scriptfield.yml | 3 +- server/build.gradle | 12 +- .../index/fielddata/ScriptDocValues.java | 44 +++- .../elasticsearch/script/ScriptModule.java | 12 ++ .../fielddata/ScriptDocValuesDatesTests.java | 11 +- .../fielddata/ScriptDocValuesLongsTests.java | 17 +- ...criptDocValuesMissingV6BehaviourTests.java | 195 ++++++++++++++++++ .../search/functionscore/FunctionScoreIT.java | 9 +- 11 files changed, 311 insertions(+), 19 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesMissingV6BehaviourTests.java diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy index 0123abd7ce104..ac1e88ba55cee 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy @@ -691,6 +691,7 @@ class BuildPlugin implements Plugin { systemProperty 'tests.task', path systemProperty 'tests.security.manager', 'true' systemProperty 'jna.nosys', 'true' + systemProperty 'es.scripting.exception_for_missing_value', 'true' // TODO: remove setting logging level via system property systemProperty 'tests.logger.level', 'WARN' for (Map.Entry property : System.properties.entrySet()) { diff --git a/docs/painless/painless-getting-started.asciidoc b/docs/painless/painless-getting-started.asciidoc index b47b417c793e5..02435b0b0d5f9 100644 --- a/docs/painless/painless-getting-started.asciidoc +++ b/docs/painless/painless-getting-started.asciidoc @@ -119,6 +119,30 @@ GET hockey/_search ---------------------------------------------------------------- // CONSOLE + +[float] +===== Missing values + +If you request the value from a field `field` that isn’t in +the document, `doc['field'].value` for this document returns: + +- `0` if a `field` has a numeric datatype (long, double etc.) +- `false` is a `field` has a boolean datatype +- epoch date if a `field` has a date datatype +- `null` if a `field` has a string datatype +- `null` if a `field` has a geo datatype +- `""` if a `field` has a binary datatype + +IMPORTANT: Starting in 7.0, `doc['field'].value` throws an exception if +the field is missing in a document. To enable this behavior now, +set a {ref}/jvm-options.html[`jvm.option`] +`-Des.scripting.exception_for_missing_value=true` on a node. If you do not enable +this behavior, a deprecation warning is logged on start up. + +To check if a document is missing a value, you can call +`doc['field'].size() == 0`. + + [float] ==== Updating Fields with Painless diff --git a/modules/lang-painless/build.gradle b/modules/lang-painless/build.gradle index d287d7ee02378..fb1ea441a9dd5 100644 --- a/modules/lang-painless/build.gradle +++ b/modules/lang-painless/build.gradle @@ -17,7 +17,7 @@ * under the License. */ -import org.apache.tools.ant.types.Path + esplugin { description 'An easy, safe and fast scripting language for Elasticsearch' diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/20_scriptfield.yml b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/20_scriptfield.yml index e498a1737576e..69b5bb132a5ee 100644 --- a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/20_scriptfield.yml +++ b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/20_scriptfield.yml @@ -88,14 +88,13 @@ setup: --- "Scripted Field with a null safe dereference (null)": - # Change this to ?: once we have it implemented - do: search: body: script_fields: bar: script: - source: "(doc['missing'].value?.length() ?: 0) + params.x;" + source: "(doc['missing'].size() == 0 ? 0 : doc['missing'].value.length()) + params.x;" params: x: 5 diff --git a/server/build.gradle b/server/build.gradle index 3055c625ea914..da60bca5a3e81 100644 --- a/server/build.gradle +++ b/server/build.gradle @@ -329,7 +329,7 @@ if (isEclipse == false || project.path == ":server-tests") { task integTest(type: RandomizedTestingTask, group: JavaBasePlugin.VERIFICATION_GROUP, description: 'Multi-node tests', - dependsOn: test.dependsOn) { + dependsOn: test.dependsOn.collect()) { configure(BuildPlugin.commonTestConfig(project)) classpath = project.test.classpath testClassesDirs = project.test.testClassesDirs @@ -338,3 +338,13 @@ if (isEclipse == false || project.path == ":server-tests") { check.dependsOn integTest integTest.mustRunAfter test } + +// TODO: remove ScriptDocValuesMissingV6BehaviourTests in 7.0 +additionalTest('testScriptDocValuesMissingV6Behaviour'){ + include '**/ScriptDocValuesMissingV6BehaviourTests.class' + systemProperty 'es.scripting.exception_for_missing_value', 'false' +} +test { + // these are tested explicitly in separate test tasks + exclude '**/*ScriptDocValuesMissingV6BehaviourTests.class' +} diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java index 3d07a0f87aa5e..7664345bd46e2 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java @@ -29,6 +29,7 @@ import org.elasticsearch.common.geo.GeoUtils; import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.logging.ESLoggerFactory; +import org.elasticsearch.script.ScriptModule; import org.joda.time.DateTime; import org.joda.time.DateTimeZone; import org.joda.time.MutableDateTime; @@ -154,6 +155,10 @@ public SortedNumericDocValues getInternalValues() { public long getValue() { if (count == 0) { + if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) { + throw new IllegalStateException("A document doesn't have a value for a field! " + + "Use doc[].size()==0 to check if a document is missing a field!"); + } return 0L; } return values[0]; @@ -246,6 +251,10 @@ public Dates(SortedNumericDocValues in) { */ public ReadableDateTime getValue() { if (count == 0) { + if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) { + throw new IllegalStateException("A document doesn't have a value for a field! " + + "Use doc[].size()==0 to check if a document is missing a field!"); + } return EPOCH; } return get(0); @@ -381,6 +390,10 @@ public SortedNumericDoubleValues getInternalValues() { public double getValue() { if (count == 0) { + if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) { + throw new IllegalStateException("A document doesn't have a value for a field! " + + "Use doc[].size()==0 to check if a document is missing a field!"); + } return 0d; } return values[0]; @@ -437,6 +450,10 @@ protected void resize(int newSize) { public GeoPoint getValue() { if (count == 0) { + if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) { + throw new IllegalStateException("A document doesn't have a value for a field! " + + "Use doc[].size()==0 to check if a document is missing a field!"); + } return null; } return values[0]; @@ -549,7 +566,14 @@ protected void resize(int newSize) { } public boolean getValue() { - return count != 0 && values[0]; + if (count == 0) { + if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) { + throw new IllegalStateException("A document doesn't have a value for a field! " + + "Use doc[].size()==0 to check if a document is missing a field!"); + } + return false; + } + return values[0]; } @Override @@ -632,7 +656,14 @@ public String get(int index) { } public String getValue() { - return count == 0 ? null : get(0); + if (count == 0) { + if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) { + throw new IllegalStateException("A document doesn't have a value for a field! " + + "Use doc[].size()==0 to check if a document is missing a field!"); + } + return null; + } + return get(0); } } @@ -653,7 +684,14 @@ public BytesRef get(int index) { } public BytesRef getValue() { - return count == 0 ? new BytesRef() : get(0); + if (count == 0) { + if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) { + throw new IllegalStateException("A document doesn't have a value for a field! " + + "Use doc[].size()==0 to check if a document is missing a field!"); + } + return new BytesRef(); + } + return get(0); } } diff --git a/server/src/main/java/org/elasticsearch/script/ScriptModule.java b/server/src/main/java/org/elasticsearch/script/ScriptModule.java index f0e075eac7d93..042953117c5a5 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptModule.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptModule.java @@ -31,6 +31,9 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.plugins.ScriptPlugin; import org.elasticsearch.search.aggregations.pipeline.movfn.MovingFunctionScript; +import org.elasticsearch.common.Booleans; +import org.elasticsearch.common.logging.DeprecationLogger; +import org.elasticsearch.common.logging.Loggers; /** * Manages building {@link ScriptService}. @@ -61,6 +64,11 @@ public class ScriptModule { ).collect(Collectors.toMap(c -> c.name, Function.identity())); } + public static final boolean EXCEPTION_FOR_MISSING_VALUE = + Booleans.parseBoolean(System.getProperty("es.scripting.exception_for_missing_value", "false")); + + private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(ScriptModule.class)); + private final ScriptService scriptService; public ScriptModule(Settings settings, List scriptPlugins) { @@ -84,6 +92,10 @@ public ScriptModule(Settings settings, List scriptPlugins) { } } } + if (EXCEPTION_FOR_MISSING_VALUE == false) + DEPRECATION_LOGGER.deprecated("Script: returning default values for missing document values is deprecated. " + + "Set system property '-Des.scripting.exception_for_missing_value=true' " + + "to make behaviour compatible with future major versions."); scriptService = new ScriptService(settings, Collections.unmodifiableMap(engines), Collections.unmodifiableMap(contexts)); } diff --git a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java index 7a0a6816a66bf..4244bfc564d03 100644 --- a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java +++ b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java @@ -61,9 +61,14 @@ public void test() throws IOException { for (int round = 0; round < 10; round++) { int d = between(0, values.length - 1); dates.setNextDocId(d); - assertEquals(expectedDates[d].length > 0 ? expectedDates[d][0] : new DateTime(0, DateTimeZone.UTC), dates.getValue()); - assertEquals(expectedDates[d].length > 0 ? expectedDates[d][0] : new DateTime(0, DateTimeZone.UTC), dates.getDate()); - + if (expectedDates[d].length > 0) { + assertEquals(expectedDates[d][0] , dates.getValue()); + assertEquals(expectedDates[d][0] , dates.getDate()); + } else { + Exception e = expectThrows(IllegalStateException.class, () -> dates.getValue()); + assertEquals("A document doesn't have a value for a field! " + + "Use doc[].size()==0 to check if a document is missing a field!", e.getMessage()); + } assertEquals(values[d].length, dates.size()); for (int i = 0; i < values[d].length; i++) { assertEquals(expectedDates[d][i], dates.get(i)); diff --git a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java index 8b20e9a9f3a71..166f3d8ccd272 100644 --- a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java +++ b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java @@ -52,8 +52,13 @@ public void testLongs() throws IOException { for (int round = 0; round < 10; round++) { int d = between(0, values.length - 1); longs.setNextDocId(d); - assertEquals(values[d].length > 0 ? values[d][0] : 0, longs.getValue()); - + if (values[d].length > 0) { + assertEquals(values[d][0], longs.getValue()); + } else { + Exception e = expectThrows(IllegalStateException.class, () -> longs.getValue()); + assertEquals("A document doesn't have a value for a field! " + + "Use doc[].size()==0 to check if a document is missing a field!", e.getMessage()); + } assertEquals(values[d].length, longs.size()); assertEquals(values[d].length, longs.getValues().size()); for (int i = 0; i < values[d].length; i++) { @@ -88,7 +93,13 @@ public void testDates() throws IOException { for (int round = 0; round < 10; round++) { int d = between(0, values.length - 1); longs.setNextDocId(d); - assertEquals(dates[d].length > 0 ? dates[d][0] : new DateTime(0, DateTimeZone.UTC), longs.getDate()); + if (dates[d].length > 0) { + assertEquals(dates[d][0], longs.getDate()); + } else { + Exception e = expectThrows(IllegalStateException.class, () -> longs.getDate()); + assertEquals("A document doesn't have a value for a field! " + + "Use doc[].size()==0 to check if a document is missing a field!", e.getMessage()); + } assertEquals(values[d].length, longs.getDates().size()); for (int i = 0; i < values[d].length; i++) { diff --git a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesMissingV6BehaviourTests.java b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesMissingV6BehaviourTests.java new file mode 100644 index 0000000000000..1dc836874d847 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesMissingV6BehaviourTests.java @@ -0,0 +1,195 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.index.fielddata; + +import org.elasticsearch.common.geo.GeoPoint; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.fielddata.ScriptDocValues.Longs; +import org.elasticsearch.index.fielddata.ScriptDocValues.Dates; +import org.elasticsearch.index.fielddata.ScriptDocValues.Booleans; +import org.elasticsearch.plugins.ScriptPlugin; +import org.elasticsearch.script.MockScriptEngine; +import org.elasticsearch.script.ScriptContext; +import org.elasticsearch.script.ScriptEngine; +import org.elasticsearch.script.ScriptModule; +import org.elasticsearch.test.ESTestCase; + +import org.joda.time.DateTime; +import org.joda.time.DateTimeZone; +import org.joda.time.ReadableDateTime; +import java.io.IOException; +import java.util.Collection; +import java.util.Collections; + +import static java.util.Collections.singletonList; + +public class ScriptDocValuesMissingV6BehaviourTests extends ESTestCase { + + public void testScriptMissingValuesWarning(){ + new ScriptModule(Settings.EMPTY, singletonList(new ScriptPlugin() { + @Override + public ScriptEngine getScriptEngine(Settings settings, Collection> contexts) { + return new MockScriptEngine(MockScriptEngine.NAME, Collections.singletonMap("1", script -> "1")); + } + })); + assertWarnings("Script: returning default values for missing document values is deprecated. " + + "Set system property '-Des.scripting.exception_for_missing_value=true' " + + "to make behaviour compatible with future major versions."); + } + + public void testZeroForMissingValueLong() throws IOException { + long[][] values = new long[between(3, 10)][]; + for (int d = 0; d < values.length; d++) { + values[d] = new long[0]; + } + Longs longs = wrap(values); + for (int round = 0; round < 10; round++) { + int d = between(0, values.length - 1); + longs.setNextDocId(d); + assertEquals(0, longs.getValue()); + } + } + + public void testEpochForMissingValueDate() throws IOException { + final ReadableDateTime EPOCH = new DateTime(0, DateTimeZone.UTC); + long[][] values = new long[between(3, 10)][]; + for (int d = 0; d < values.length; d++) { + values[d] = new long[0]; + } + Dates dates = wrapDates(values); + for (int round = 0; round < 10; round++) { + int d = between(0, values.length - 1); + dates.setNextDocId(d); + assertEquals(EPOCH, dates.getValue()); + } + } + + public void testFalseForMissingValueBoolean() throws IOException { + long[][] values = new long[between(3, 10)][]; + for (int d = 0; d < values.length; d++) { + values[d] = new long[0]; + } + Booleans bools = wrapBooleans(values); + for (int round = 0; round < 10; round++) { + int d = between(0, values.length - 1); + bools.setNextDocId(d); + assertEquals(false, bools.getValue()); + } + } + + public void testNullForMissingValueGeo() throws IOException{ + final MultiGeoPointValues values = wrap(new GeoPoint[0]); + final ScriptDocValues.GeoPoints script = new ScriptDocValues.GeoPoints(values); + script.setNextDocId(0); + assertEquals(null, script.getValue()); + } + + + private Longs wrap(long[][] values) { + return new Longs(new AbstractSortedNumericDocValues() { + long[] current; + int i; + @Override + public boolean advanceExact(int doc) { + i = 0; + current = values[doc]; + return current.length > 0; + } + @Override + public int docValueCount() { + return current.length; + } + @Override + public long nextValue() { + return current[i++]; + } + }); + } + + private Booleans wrapBooleans(long[][] values) { + return new Booleans(new AbstractSortedNumericDocValues() { + long[] current; + int i; + @Override + public boolean advanceExact(int doc) { + i = 0; + current = values[doc]; + return current.length > 0; + } + @Override + public int docValueCount() { + return current.length; + } + @Override + public long nextValue() { + return current[i++]; + } + }); + } + + private Dates wrapDates(long[][] values) { + return new Dates(new AbstractSortedNumericDocValues() { + long[] current; + int i; + @Override + public boolean advanceExact(int doc) { + current = values[doc]; + i = 0; + return current.length > 0; + } + @Override + public int docValueCount() { + return current.length; + } + @Override + public long nextValue() { + return current[i++]; + } + }); + } + + + private static MultiGeoPointValues wrap(final GeoPoint... points) { + return new MultiGeoPointValues() { + int docID = -1; + int i; + @Override + public GeoPoint nextValue() { + if (docID != 0) { + fail(); + } + return points[i++]; + } + @Override + public boolean advanceExact(int docId) { + docID = docId; + return points.length > 0; + } + @Override + public int docValueCount() { + if (docID != 0) { + return 0; + } + return points.length; + } + }; + } + +} diff --git a/server/src/test/java/org/elasticsearch/search/functionscore/FunctionScoreIT.java b/server/src/test/java/org/elasticsearch/search/functionscore/FunctionScoreIT.java index 0e92aba2a8552..12e48a3ae4f0a 100644 --- a/server/src/test/java/org/elasticsearch/search/functionscore/FunctionScoreIT.java +++ b/server/src/test/java/org/elasticsearch/search/functionscore/FunctionScoreIT.java @@ -134,15 +134,12 @@ public void testScriptScoresWithAgg() throws IOException { } public void testMinScoreFunctionScoreBasic() throws IOException { - index(INDEX, TYPE, jsonBuilder().startObject().field("num", 2).endObject()); - refresh(); float score = randomFloat(); float minScore = randomFloat(); - index(INDEX, TYPE, jsonBuilder().startObject() - .field("num", 2) - .field("random_score", score) // Pass the random score as a document field so that it can be extracted in the script - .endObject()); + .field("num", 2) + .field("random_score", score) // Pass the random score as a document field so that it can be extracted in the script + .endObject()); refresh(); ensureYellow();