From 4c68dfe0016673bbdb1885384d1934858180d521 Mon Sep 17 00:00:00 2001 From: Mayya Sharipova Date: Thu, 19 Jul 2018 17:41:06 -0400 Subject: [PATCH] Handle missing values in painless (#32207) Throw an exception for doc['field'].value if this document is missing a value for the field. After deprecation changes have been backported to 6.x, make this a default behaviour in 7.0 Closes #29286 --- .../elasticsearch/gradle/BuildPlugin.groovy | 1 - .../painless-getting-started.asciidoc | 17 +- server/build.gradle | 10 - .../index/fielddata/ScriptDocValues.java | 44 +--- .../elasticsearch/script/ScriptModule.java | 13 +- ...criptDocValuesMissingV6BehaviourTests.java | 195 ------------------ 6 files changed, 14 insertions(+), 266 deletions(-) delete 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 219d00ba640..c5dd19de3cc 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy @@ -750,7 +750,6 @@ 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 887769e49ab..1dec4a33bb5 100644 --- a/docs/painless/painless-getting-started.asciidoc +++ b/docs/painless/painless-getting-started.asciidoc @@ -123,21 +123,8 @@ GET hockey/_search [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. +`doc['field'].value` throws an exception if +the field is missing in a document. To check if a document is missing a value, you can call `doc['field'].size() == 0`. diff --git a/server/build.gradle b/server/build.gradle index 7db073f43a5..c71cc4c7dbd 100644 --- a/server/build.gradle +++ b/server/build.gradle @@ -156,16 +156,6 @@ if (isEclipse) { compileJava.options.compilerArgs << "-Xlint:-cast,-deprecation,-rawtypes,-try,-unchecked" compileTestJava.options.compilerArgs << "-Xlint:-cast,-deprecation,-rawtypes,-try,-unchecked" -// 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' -} - forbiddenPatterns { exclude '**/*.json' exclude '**/*.jmx' 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 6d888bd63e3..fedad6e134b 100644 --- a/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java +++ b/server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java @@ -29,7 +29,6 @@ import org.elasticsearch.common.geo.GeoPoint; 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; @@ -126,11 +125,8 @@ public abstract class ScriptDocValues extends AbstractList { 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; + 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 values[0]; } @@ -172,11 +168,8 @@ public abstract class ScriptDocValues extends AbstractList { */ 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; + 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 get(0); } @@ -277,11 +270,8 @@ public abstract class ScriptDocValues extends AbstractList { 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; + 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 values[0]; } @@ -337,11 +327,8 @@ public abstract class ScriptDocValues extends AbstractList { 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! " + + 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]; } @@ -454,11 +441,8 @@ public abstract class ScriptDocValues extends AbstractList { public boolean 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 false; + 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 values[0]; } @@ -544,11 +528,8 @@ public abstract class ScriptDocValues extends AbstractList { public String getValue() { if (count == 0) { - if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) { - throw new IllegalStateException("A document doesn't have a value for a field! " + + 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); } @@ -572,11 +553,8 @@ public abstract class ScriptDocValues extends AbstractList { public BytesRef getValue() { if (count == 0) { - if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) { - throw new IllegalStateException("A document doesn't have a value for a field! " + + 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 bf4bd9c57ce..a3da1dafe48 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptModule.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptModule.java @@ -31,9 +31,7 @@ import org.elasticsearch.common.settings.ClusterSettings; 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}. @@ -64,11 +62,6 @@ 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) { @@ -92,10 +85,6 @@ public class ScriptModule { } } } - 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/ScriptDocValuesMissingV6BehaviourTests.java b/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesMissingV6BehaviourTests.java deleted file mode 100644 index 1dc836874d8..00000000000 --- a/server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesMissingV6BehaviourTests.java +++ /dev/null @@ -1,195 +0,0 @@ -/* - * 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; - } - }; - } - -}