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

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

@@ -750,7 +750,6 @@ class BuildPlugin implements Plugin<Project> {
             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<String, String> property : System.properties.entrySet()) {

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
+`doc['field'].value` throws an exception if
-the document, `doc['field'].value` for this document returns:
+the field is missing in a document.
-
-- `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`.

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'

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<T> extends AbstractList<T> {
 
         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! " +
-                    throw new IllegalStateException("A document doesn't have a value for a field! " +
+                    "Use doc[<field>].size()==0 to check if a document is missing a field!");
-                        "Use doc[<field>].size()==0 to check if a document is missing a field!");
-                }
-                return 0L;
             }
             return values[0];
         }
@@ -172,11 +168,8 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
          */
         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! " +
-                    throw new IllegalStateException("A document doesn't have a value for a field! " +
+                    "Use doc[<field>].size()==0 to check if a document is missing a field!");
-                        "Use doc[<field>].size()==0 to check if a document is missing a field!");
-                }
-                return EPOCH;
             }
             return get(0);
         }
@@ -277,11 +270,8 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
 
         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! " +
-                    throw new IllegalStateException("A document doesn't have a value for a field! " +
+                   "Use doc[<field>].size()==0 to check if a document is missing a field!");
-                        "Use doc[<field>].size()==0 to check if a document is missing a field!");
-                }
-                return 0d;
             }
             return values[0];
         }
@@ -337,11 +327,8 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
 
         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[<field>].size()==0 to check if a document is missing a field!");
-                }
-                return null;
             }
             return values[0];
         }
@@ -454,11 +441,8 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
 
         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! " +
-                    throw new IllegalStateException("A document doesn't have a value for a field! " +
+                    "Use doc[<field>].size()==0 to check if a document is missing a field!");
-                        "Use doc[<field>].size()==0 to check if a document is missing a field!");
-                }
-                return false;
             }
             return values[0];
         }
@@ -544,11 +528,8 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
 
         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[<field>].size()==0 to check if a document is missing a field!");
-                }
-                return null;
             }
             return get(0);
         }
@@ -572,11 +553,8 @@ public abstract class ScriptDocValues<T> extends AbstractList<T> {
 
         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[<field>].size()==0 to check if a document is missing a field!");
-                }
-                return new BytesRef();
             }
             return get(0);
         }

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<ScriptPlugin> 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));
     }
 

server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesMissingV6BehaviourTests.java

@@ -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<ScriptContext<?>> 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;
-            }
-        };
-    }
-
-}