Skip to content

Commit 94d8823

Browse files
mayya-sharipovamayya-sharipova
authored
Handle missing values in painless (#30975) (#31903)
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
1 parent 34e7858 commit 94d8823

File tree

11 files changed

+311
-19
lines changed

11 files changed

+311
-19
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -691,6 +691,7 @@ class BuildPlugin implements Plugin<Project> {
691691
systemProperty 'tests.task', path
692692
systemProperty 'tests.security.manager', 'true'
693693
systemProperty 'jna.nosys', 'true'
694+
systemProperty 'es.scripting.exception_for_missing_value', 'true'
694695
// TODO: remove setting logging level via system property
695696
systemProperty 'tests.logger.level', 'WARN'
696697
for (Map.Entry<String, String> property : System.properties.entrySet()) {

‎docs/painless/painless-getting-started.asciidoc‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,30 @@ GET hockey/_search
119119
----------------------------------------------------------------
120120
// CONSOLE
121121

122+
123+
[float]
124+
===== Missing values
125+
126+
If you request the value from a field `field` that isn’t in
127+
the document, `doc['field'].value` for this document returns:
128+
129+
- `0` if a `field` has a numeric datatype (long, double etc.)
130+
- `false` is a `field` has a boolean datatype
131+
- epoch date if a `field` has a date datatype
132+
- `null` if a `field` has a string datatype
133+
- `null` if a `field` has a geo datatype
134+
- `""` if a `field` has a binary datatype
135+
136+
IMPORTANT: Starting in 7.0, `doc['field'].value` throws an exception if
137+
the field is missing in a document. To enable this behavior now,
138+
set a {ref}/jvm-options.html[`jvm.option`]
139+
`-Des.scripting.exception_for_missing_value=true` on a node. If you do not enable
140+
this behavior, a deprecation warning is logged on start up.
141+
142+
To check if a document is missing a value, you can call
143+
`doc['field'].size() == 0`.
144+
145+
122146
[float]
123147
==== Updating Fields with Painless
124148

‎modules/lang-painless/build.gradle‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
* under the License.
1818
*/
1919

20-
import org.apache.tools.ant.types.Path
20+
2121

2222
esplugin {
2323
description 'An easy, safe and fast scripting language for Elasticsearch'

‎modules/lang-painless/src/test/resources/rest-api-spec/test/painless/20_scriptfield.yml‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,14 +88,13 @@ setup:
8888

8989
---
9090
"Scripted Field with a null safe dereference (null)":
91-
# Change this to ?: once we have it implemented
9291
- do:
9392
search:
9493
body:
9594
script_fields:
9695
bar:
9796
script:
98-
source: "(doc['missing'].value?.length() ?: 0) + params.x;"
97+
source: "(doc['missing'].size() == 0 ? 0 : doc['missing'].value.length()) + params.x;"
9998
params:
10099
x: 5
101100

‎server/build.gradle‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -339,7 +339,7 @@ if (isEclipse == false || project.path == ":server-tests") {
339339
task integTest(type: RandomizedTestingTask,
340340
group: JavaBasePlugin.VERIFICATION_GROUP,
341341
description: 'Multi-node tests',
342-
dependsOn: test.dependsOn) {
342+
dependsOn: test.dependsOn.collect()) {
343343
configure(BuildPlugin.commonTestConfig(project))
344344
classpath = project.test.classpath
345345
testClassesDirs = project.test.testClassesDirs
@@ -348,3 +348,13 @@ if (isEclipse == false || project.path == ":server-tests") {
348348
check.dependsOn integTest
349349
integTest.mustRunAfter test
350350
}
351+
352+
// TODO: remove ScriptDocValuesMissingV6BehaviourTests in 7.0
353+
additionalTest('testScriptDocValuesMissingV6Behaviour'){
354+
include '**/ScriptDocValuesMissingV6BehaviourTests.class'
355+
systemProperty 'es.scripting.exception_for_missing_value', 'false'
356+
}
357+
test {
358+
// these are tested explicitly in separate test tasks
359+
exclude '**/*ScriptDocValuesMissingV6BehaviourTests.class'
360+
}

‎server/src/main/java/org/elasticsearch/index/fielddata/ScriptDocValues.java‎

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import org.elasticsearch.common.geo.GeoUtils;
3030
import org.elasticsearch.common.logging.DeprecationLogger;
3131
import org.elasticsearch.common.logging.ESLoggerFactory;
32+
import org.elasticsearch.script.ScriptModule;
3233
import org.joda.time.DateTime;
3334
import org.joda.time.DateTimeZone;
3435
import org.joda.time.MutableDateTime;
@@ -154,6 +155,10 @@ public SortedNumericDocValues getInternalValues() {
154155

155156
public long getValue() {
156157
if (count == 0) {
158+
if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) {
159+
throw new IllegalStateException("A document doesn't have a value for a field! " +
160+
"Use doc[<field>].size()==0 to check if a document is missing a field!");
161+
}
157162
return 0L;
158163
}
159164
return values[0];
@@ -246,6 +251,10 @@ public Dates(SortedNumericDocValues in) {
246251
*/
247252
public ReadableDateTime getValue() {
248253
if (count == 0) {
254+
if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) {
255+
throw new IllegalStateException("A document doesn't have a value for a field! " +
256+
"Use doc[<field>].size()==0 to check if a document is missing a field!");
257+
}
249258
return EPOCH;
250259
}
251260
return get(0);
@@ -381,6 +390,10 @@ public SortedNumericDoubleValues getInternalValues() {
381390

382391
public double getValue() {
383392
if (count == 0) {
393+
if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) {
394+
throw new IllegalStateException("A document doesn't have a value for a field! " +
395+
"Use doc[<field>].size()==0 to check if a document is missing a field!");
396+
}
384397
return 0d;
385398
}
386399
return values[0];
@@ -437,6 +450,10 @@ protected void resize(int newSize) {
437450

438451
public GeoPoint getValue() {
439452
if (count == 0) {
453+
if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) {
454+
throw new IllegalStateException("A document doesn't have a value for a field! " +
455+
"Use doc[<field>].size()==0 to check if a document is missing a field!");
456+
}
440457
return null;
441458
}
442459
return values[0];
@@ -549,7 +566,14 @@ protected void resize(int newSize) {
549566
}
550567

551568
public boolean getValue() {
552-
return count != 0 && values[0];
569+
if (count == 0) {
570+
if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) {
571+
throw new IllegalStateException("A document doesn't have a value for a field! " +
572+
"Use doc[<field>].size()==0 to check if a document is missing a field!");
573+
}
574+
return false;
575+
}
576+
return values[0];
553577
}
554578

555579
@Override
@@ -632,7 +656,14 @@ public String get(int index) {
632656
}
633657

634658
public String getValue() {
635-
return count == 0 ? null : get(0);
659+
if (count == 0) {
660+
if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) {
661+
throw new IllegalStateException("A document doesn't have a value for a field! " +
662+
"Use doc[<field>].size()==0 to check if a document is missing a field!");
663+
}
664+
return null;
665+
}
666+
return get(0);
636667
}
637668
}
638669

@@ -653,7 +684,14 @@ public BytesRef get(int index) {
653684
}
654685

655686
public BytesRef getValue() {
656-
return count == 0 ? new BytesRef() : get(0);
687+
if (count == 0) {
688+
if (ScriptModule.EXCEPTION_FOR_MISSING_VALUE) {
689+
throw new IllegalStateException("A document doesn't have a value for a field! " +
690+
"Use doc[<field>].size()==0 to check if a document is missing a field!");
691+
}
692+
return new BytesRef();
693+
}
694+
return get(0);
657695
}
658696

659697
}

‎server/src/main/java/org/elasticsearch/script/ScriptModule.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@
3131
import org.elasticsearch.common.settings.Settings;
3232
import org.elasticsearch.plugins.ScriptPlugin;
3333
import org.elasticsearch.search.aggregations.pipeline.movfn.MovingFunctionScript;
34+
import org.elasticsearch.common.Booleans;
35+
import org.elasticsearch.common.logging.DeprecationLogger;
36+
import org.elasticsearch.common.logging.Loggers;
3437

3538
/**
3639
* Manages building {@link ScriptService}.
@@ -61,6 +64,11 @@ public class ScriptModule {
6164
).collect(Collectors.toMap(c -> c.name, Function.identity()));
6265
}
6366

67+
public static final boolean EXCEPTION_FOR_MISSING_VALUE =
68+
Booleans.parseBoolean(System.getProperty("es.scripting.exception_for_missing_value", "false"));
69+
70+
private static final DeprecationLogger DEPRECATION_LOGGER = new DeprecationLogger(Loggers.getLogger(ScriptModule.class));
71+
6472
private final ScriptService scriptService;
6573

6674
public ScriptModule(Settings settings, List<ScriptPlugin> scriptPlugins) {
@@ -84,6 +92,10 @@ public ScriptModule(Settings settings, List<ScriptPlugin> scriptPlugins) {
8492
}
8593
}
8694
}
95+
if (EXCEPTION_FOR_MISSING_VALUE == false)
96+
DEPRECATION_LOGGER.deprecated("Script: returning default values for missing document values is deprecated. " +
97+
"Set system property '-Des.scripting.exception_for_missing_value=true' " +
98+
"to make behaviour compatible with future major versions.");
8799
scriptService = new ScriptService(settings, Collections.unmodifiableMap(engines), Collections.unmodifiableMap(contexts));
88100
}
89101

‎server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesDatesTests.java‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,14 @@ public void test() throws IOException {
6161
for (int round = 0; round < 10; round++) {
6262
int d = between(0, values.length - 1);
6363
dates.setNextDocId(d);
64-
assertEquals(expectedDates[d].length > 0 ? expectedDates[d][0] : new DateTime(0, DateTimeZone.UTC), dates.getValue());
65-
assertEquals(expectedDates[d].length > 0 ? expectedDates[d][0] : new DateTime(0, DateTimeZone.UTC), dates.getDate());
66-
64+
if (expectedDates[d].length > 0) {
65+
assertEquals(expectedDates[d][0] , dates.getValue());
66+
assertEquals(expectedDates[d][0] , dates.getDate());
67+
} else {
68+
Exception e = expectThrows(IllegalStateException.class, () -> dates.getValue());
69+
assertEquals("A document doesn't have a value for a field! " +
70+
"Use doc[<field>].size()==0 to check if a document is missing a field!", e.getMessage());
71+
}
6772
assertEquals(values[d].length, dates.size());
6873
for (int i = 0; i < values[d].length; i++) {
6974
assertEquals(expectedDates[d][i], dates.get(i));

‎server/src/test/java/org/elasticsearch/index/fielddata/ScriptDocValuesLongsTests.java‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,13 @@ public void testLongs() throws IOException {
5252
for (int round = 0; round < 10; round++) {
5353
int d = between(0, values.length - 1);
5454
longs.setNextDocId(d);
55-
assertEquals(values[d].length > 0 ? values[d][0] : 0, longs.getValue());
56-
55+
if (values[d].length > 0) {
56+
assertEquals(values[d][0], longs.getValue());
57+
} else {
58+
Exception e = expectThrows(IllegalStateException.class, () -> longs.getValue());
59+
assertEquals("A document doesn't have a value for a field! " +
60+
"Use doc[<field>].size()==0 to check if a document is missing a field!", e.getMessage());
61+
}
5762
assertEquals(values[d].length, longs.size());
5863
assertEquals(values[d].length, longs.getValues().size());
5964
for (int i = 0; i < values[d].length; i++) {
@@ -88,7 +93,13 @@ public void testDates() throws IOException {
8893
for (int round = 0; round < 10; round++) {
8994
int d = between(0, values.length - 1);
9095
longs.setNextDocId(d);
91-
assertEquals(dates[d].length > 0 ? dates[d][0] : new DateTime(0, DateTimeZone.UTC), longs.getDate());
96+
if (dates[d].length > 0) {
97+
assertEquals(dates[d][0], longs.getDate());
98+
} else {
99+
Exception e = expectThrows(IllegalStateException.class, () -> longs.getDate());
100+
assertEquals("A document doesn't have a value for a field! " +
101+
"Use doc[<field>].size()==0 to check if a document is missing a field!", e.getMessage());
102+
}
92103

93104
assertEquals(values[d].length, longs.getDates().size());
94105
for (int i = 0; i < values[d].length; i++) {

0 commit comments

Comments
 (0)