Handle missing and multiple values in script #29611

mayya-sharipova · 2018-04-19T13:24:35Z

Previously in script for numeric fields, there was no way to check if
a document is missing a value. Also certain operations on multiple-
values fields were missing.

This PR adds the following:

return null for doc['field'] if a document is missing a 'field1':
Now we can do this:
if (doc['field'] == null) {return -1;} return doc['field'].value; or
doc['field']?.value ?: -1
add the following functions for multiple-valued numeric fields:
doc['field'].min returns the minumum amoung values
doc['field'].max returns the maximum amoung values
doc['field'].sum returns the sum of amoung values
doc['field'].avg returns the average of values

Closes #29286

Previously in script for numeric fields, there was no way to check if a document is missing a value. Also certain operations on multiple- values fields were missing. This PR adds the following: - return null for doc['field'] if a document is missing a 'field1': Now we can do this: if (doc['field'] == null) {return -1;} return doc['field'].value; or doc['field']?.value ?: -1 - add the following functions for multiple-valued numeric fields: doc['field'].min returns the minumum amoung values doc['field'].max returns the maximum amoung values doc['field'].sum returns the sum of amoung values doc['field'].avg returns the average of values Closes elastic#29286

elasticmachine · 2018-04-20T18:47:16Z

Pinging @elastic/es-core-infra

jdconrad

Hey Mayya, thanks for tackling this. The changes seem good. My one concern is this is a backwards incompatible change. Users currently may not be checking the fields for existence and will suddenly get NPE's if they are using the default values for docs with missing fields. One approach to solve this problem would be to have a setting that allows users to turn this missing field return null on in 6.x. If the flag is not set, we give a deprecation warning. Then remove the flag altogether in 7.

mayya-sharipova · 2018-04-25T12:04:01Z

@jdconrad Thanks Jack for your review. Your concern makes sense.

Do you think we should add an index-level setting for 6.x:

"painless.missing_value.null" : true

The default will be false.
And are you suggesting if this setting is not set, while executing script for all null values, we will output a deprecation warning?

@rjernst what do you think?

rjernst · 2018-04-25T23:51:24Z

I think doing a deprecation either through a setting or a sysprop is a good idea. Deprecating getDate() has been a pain and we have ended up with sometimes flakey tests because we did deprecation logging inside ScriptDocValues.

If doing through a sysprop, we have the advantage of the code change being very simple. We would just need one place somewhere during node startup that checked what value we statically read, and emitting a deprecation log message if it is set to the old behavior.

For a setting, I think we would want this as a cluster setting, since scripts can be run across many indices, and it would not make sense (or be possible, since scripts are compiled once for the node) to vary this per index/shard. However, passing the cluster setting down and updating may be tricky. At least, it will require a few levels of signature changes to IndicesService, QueryShardContext and SearchLookup classes. I think we would want to get a BooleanSupplier down into LeafDocLookup, for which the return value is tied to this cluster setting. This is tricky because we don't currently have ClusterSettings (which is needed to hook up a settings listener) in IndicesService.

As far as when we should emit the deprecation warning, I think in either case (sysprop or cluster setting) it should be during startup.

jdconrad · 2018-04-26T17:23:47Z

I agree on the one time deprecation warning on start up. Otherwise the logs get filled with a huge amount of spam whenever a script is used.

Previously in script for numeric fields, there was no way to check if a document is missing a value. Also certain operations on multiple- values fields were missing. This PR adds the following: - add the following functions for multiple-valued numeric fields: doc['field'].min returns the minumum amoung values doc['field'].max returns the maximum amoung values doc['field'].sum returns the sum of amoung values doc['field'].avg returns the average of values - return null for doc['field'] if a document is missing a 'field1': Now we can do this: if (doc['field'] == null) {return -1;} return doc['field'].value; or doc['field']?.value ?: -1 This new behaviour will only work if the following system property is set: `export ES_JAVA_OPTS="-Des.script.null_for_missing_value=true"' Closes elastic#29286

…ple-values-in-script

jdconrad

LGTM. Thanks again for doing this!

rjernst

This looks pretty good. I have some requests.

rjernst · 2018-05-14T14:29:57Z

docs/CHANGELOG.asciidoc

@@ -19,6 +19,7 @@
 === Breaking Java Changes

 === Deprecations
+Returning 0 for missing numeric fields in script is deprecated. PR: 29611


You can remove this, since the changelog is being taken another direction.

rjernst · 2018-05-14T14:31:25Z

docs/painless/painless-getting-started.asciidoc

+about this forthcoming change.
+
+
+===== Multiple values


I'm not sure that the getting started guide is the place for either of these things to be documented. Maybe @debadair has a better suggestion for where they could fit in?

I feel like the "examples" section is really the "getting started" guide, and the other pieces should probably be split out. Not sure we want to do that as part of this change, though. For now, I'd rename this Multi-value field operations , put it after the regular expressions section, and bump it up to a level 3 heading so it's a separate page.

rjernst · 2018-05-14T14:35:56Z

docs/painless/painless-getting-started.asciidoc

+will be changed in the next major version of elasticsearch:
+if a document is missing a field `field`, `doc['field']` for this document
+will return `null`. You can set a system property
+`export ES_JAVA_OPTS="-Des.script.null_for_missing_value=true"' on a node


We should link to setting jvm.options, since that is OS agnostic.

rjernst · 2018-05-14T14:43:40Z

docs/painless/painless-getting-started.asciidoc

+[float]
+===== Missing values
+
+Currently by default, if a document is missing a numeric field `field`,


I would describe this a bit differently. I suggest using 2 different paragraphs. The first should describe the current behavior (but it is not necessary to say "currently by default"). This should also not state "returns 0" as it is field type dependent what is returned (and all field types behavior should be described).

The next paragraph can then describe the new option. The last sentence can mention the default behavior will change in a future release.

I think it makes sense to keep the missing values info in the Accessing doc values section. I'd probably pare it down to something like:

If you request the value from a numeric field that isn’t in the document, Painless returns zero.

IMPORTANT: Starting in 7.0, Painless returns a value of null instead of zero if the field does not exist. To enable this behavior now, set the following system property:
+
export ES_JAVA_OPTS="-Des.script.null_for_missing_value=true”
+
If you do not enable this behavior, a deprecation warning is logged on start up.

rjernst · 2018-05-14T14:45:18Z

modules/lang-painless/build.gradle

@@ -25,6 +25,7 @@ esplugin {
 }

 integTestCluster {
+  systemProperty 'es.script.null_for_missing_value', 'true'


We should have another integ test where this value is left out, to ensure the old behavior still works.

@rjernst Thanks for the feedback. What is a proper way to create another integ test?

I have got the following code, but I am not sure how to add module mapper-extras to additionalClusterTestCluster as well.

integTestCluster { module project.project(':modules:mapper-extras') } Task additionalClusterTest = tasks.create( name: "additionalClusterTest", type: RestIntegTestTask) additionalClusterTestCluster { systemProperty 'es.script.null_for_missing_value', 'true' }

I am not sure how to add module mapper-extras to additionalClusterTestCluster

There is not need to add it, since rest integ tests run against the entire distribution (ie it will have all modules).

However, it would be better to do it. So, use this for the cluster config:

systemProperty 'es.script.null_for_missing_value', 'true' distribution = 'integ-test-zip' module project // add the lang-painless module itself module project.project(':modules:mapper-extras')

@rjernst thanks a lot for the suggestion

rjernst · 2018-05-14T14:46:28Z

server/src/main/java/org/elasticsearch/node/Node.java

@@ -718,6 +718,10 @@ public void onTimeout(TimeValue timeout) {
            writePortsFile("transport", transport.boundAddress());
        }

+        if (!ScriptModule.NULL_FOR_MISSING_VALUE)


== false please

rjernst · 2018-05-14T14:47:03Z

server/src/main/java/org/elasticsearch/node/Node.java

@@ -718,6 +718,10 @@ public void onTimeout(TimeValue timeout) {
            writePortsFile("transport", transport.boundAddress());
        }

+        if (!ScriptModule.NULL_FOR_MISSING_VALUE)
+            logger.warn("Script: returning 0 for missing numeric values is deprecated. " +


This should be using a DeprecationLogger, so the message goes to the deprecation log.

rjernst · 2018-05-14T14:47:34Z

server/src/main/java/org/elasticsearch/search/lookup/LeafDocLookup.java

@@ -91,7 +92,8 @@ public void setDocument(int docId) {
            localCacheFieldData.put(fieldName, scriptValues);
        }
        try {
-            scriptValues.setNextDocId(docId);
+            boolean docHasValues = scriptValues.setNextDocId(docId);
+            if (!docHasValues && ScriptModule.NULL_FOR_MISSING_VALUE) return null;


== false please

rjernst · 2018-05-14T14:48:39Z

server/src/test/java/org/elasticsearch/search/functionscore/FunctionScoreIT.java

@@ -83,7 +83,7 @@
            scripts.put("doc['random_score']", vars -> {
                Map<?, ?> doc = (Map) vars.get("doc");
                ScriptDocValues.Doubles randomScore = (ScriptDocValues.Doubles) doc.get("random_score");
-                return randomScore.getValue();
+                if (randomScore != null) {return randomScore.getValue();} else return 0;


If this is going to be on a single line, please use a ternary statement. The same applies to the other tests here that were changed in a similar way.

debadair · 2018-05-16T18:06:56Z

docs/painless/painless-getting-started.asciidoc

+[float]
+===== Missing values
+
+Currently by default, if a document is missing a numeric field `field`,


I think it makes sense to keep the missing values info in the Accessing doc values section. I'd probably pare it down to something like:

If you request the value from a numeric field that isn’t in the document, Painless returns zero.

IMPORTANT: Starting in 7.0, Painless returns a value of null instead of zero if the field does not exist. To enable this behavior now, set the following system property:
+
export ES_JAVA_OPTS="-Des.script.null_for_missing_value=true”
+
If you do not enable this behavior, a deprecation warning is logged on start up.

debadair · 2018-05-16T18:17:01Z

docs/painless/painless-getting-started.asciidoc

+about this forthcoming change.
+
+
+===== Multiple values


I feel like the "examples" section is really the "getting started" guide, and the other pieces should probably be split out. Not sure we want to do that as part of this change, though. For now, I'd rename this Multi-value field operations , put it after the regular expressions section, and bump it up to a level 3 heading so it's a separate page.

debadair · 2018-05-16T18:20:59Z

docs/painless/painless-getting-started.asciidoc

+
+There is a number of operations designed for numeric fields,
+if a document has multiple values in such a field:
+


I'd edit the intro to:

Painless supports the following operations for multi-value numeric fields:

In the list, I'd drop "among values" from min & max. For sum and average, I'd probably say "of the values" rather than "of all values".

@debadair thanks for the suggestion, noted

Previously in script for numeric fields, there was no way to check if a document is missing a value. Also certain operations on multiple- values fields were missing. This PR adds the following: - add the following functions for multiple-valued numeric fields: doc['field'].min returns the minumum amoung values doc['field'].max returns the maximum amoung values doc['field'].sum returns the sum of amoung values doc['field'].avg returns the average of values - return null for doc['field'] if a document is missing a 'field1': Now we can do this: if (doc['field'] == null) {return -1;} return doc['field'].value; or doc['field']?.value ?: -1 This new behaviour will only work if the following system property is set: `export ES_JAVA_OPTS="-Des.script.null_for_missing_value=true"'

…ple-values-in-script

mayya-sharipova · 2018-05-22T20:16:02Z

@rjernst Ryan, I have addressed all your and Deb's comments. Can you please continue the review when you have available time. Thanks.

mayya-sharipova · 2018-10-17T13:47:50Z

Closed in favour of #30975

Multiple values still need to be addressed

mayya-sharipova added the WIP label Apr 19, 2018

mayya-sharipova force-pushed the missing-and-multiple-values-in-script branch from fda0be2 to cec0419 Compare April 19, 2018 16:02

mayya-sharipova force-pushed the missing-and-multiple-values-in-script branch from cec0419 to 5aeab57 Compare April 20, 2018 16:34

mayya-sharipova removed the WIP label Apr 20, 2018

mayya-sharipova requested review from rjernst and jdconrad April 20, 2018 18:46

mayya-sharipova added the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Apr 20, 2018

mayya-sharipova added the v7.0.0 label Apr 20, 2018

colings86 added the >enhancement label Apr 24, 2018

jdconrad reviewed Apr 25, 2018

View reviewed changes

rjernst mentioned this pull request Apr 27, 2018

DocLookup has counter-intuitive behaviour #8990

Closed

mayya-sharipova added the >breaking label Apr 30, 2018

mayya-sharipova added 2 commits May 1, 2018 06:21

Merge remote-tracking branch 'upstream/master' into missing-and-multi…

acceb90

…ple-values-in-script

mayya-sharipova added the review label May 1, 2018

jdconrad approved these changes May 1, 2018

View reviewed changes

rjernst reviewed May 14, 2018

View reviewed changes

Merge branch 'master' into missing-and-multiple-values-in-script

44c2a2d

debadair reviewed May 16, 2018

View reviewed changes

mayya-sharipova added 5 commits May 16, 2018 15:09

Merge remote-tracking branch 'upstream/master' into missing-and-multi…

f5fd421

…ple-values-in-script

Correct painless-getting-started for multi values

359a000

Set system property for test framework

b0c891a

Merge remote-tracking branch 'upstream/master' into missing-and-multi…

8e71522

…ple-values-in-script

mayya-sharipova force-pushed the missing-and-multiple-values-in-script branch from 3b58d22 to 89a3bd6 Compare May 18, 2018 20:01

Set system property for tests and update painless-getting-started

3b5f421

mayya-sharipova force-pushed the missing-and-multiple-values-in-script branch from 89a3bd6 to 3b5f421 Compare May 18, 2018 20:26

Merge remote-tracking branch 'upstream/master' into missing-and-multi…

8222111

…ple-values-in-script

rjernst removed the review label Oct 10, 2018

mayya-sharipova closed this Oct 17, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle missing and multiple values in script #29611

Handle missing and multiple values in script #29611

mayya-sharipova commented Apr 19, 2018

elasticmachine commented Apr 20, 2018

jdconrad left a comment

mayya-sharipova commented Apr 25, 2018 •

edited

Loading

rjernst commented Apr 25, 2018

jdconrad commented Apr 26, 2018

jdconrad left a comment

rjernst left a comment

rjernst May 14, 2018

rjernst May 14, 2018

debadair May 16, 2018

rjernst May 14, 2018

rjernst May 14, 2018

debadair May 16, 2018

rjernst May 14, 2018

mayya-sharipova May 15, 2018

rjernst May 15, 2018

rjernst May 15, 2018 •

edited

Loading

mayya-sharipova May 15, 2018

rjernst May 14, 2018

rjernst May 14, 2018

rjernst May 14, 2018

rjernst May 14, 2018

debadair May 16, 2018

debadair May 16, 2018

debadair May 16, 2018

mayya-sharipova May 16, 2018

mayya-sharipova commented May 22, 2018

mayya-sharipova commented Oct 17, 2018


		There is a number of operations designed for numeric fields,
		if a document has multiple values in such a field:

Handle missing and multiple values in script #29611

Handle missing and multiple values in script #29611

Conversation

mayya-sharipova commented Apr 19, 2018

elasticmachine commented Apr 20, 2018

jdconrad left a comment

Choose a reason for hiding this comment

mayya-sharipova commented Apr 25, 2018 • edited Loading

rjernst commented Apr 25, 2018

jdconrad commented Apr 26, 2018

jdconrad left a comment

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayya-sharipova commented May 22, 2018

mayya-sharipova commented Oct 17, 2018

mayya-sharipova commented Apr 25, 2018 •

edited

Loading

rjernst May 15, 2018 •

edited

Loading