Bucket script aggregation throws NPE if script returns null

[davidkyle]

First some justification as the title suggests this should be expected:

I have a monotonic counter bytes_in say that figure comes from a network interface and I wish to use the derivative aggregation to find the increase per bucket. However, my counter is a 32 bit number and occasionally it overflows wrapping back to 0 at which point the derivate is a large negative number. I can filter out the misleading derivatives with the script params.bytes >= 0 ? params.bytes : null and by setting gap_policy: skip the negative derivative should be skipped.

The error occurs on master not 6.4 or earlier and appears to be a regression.

Reproduce
First index a couple of records the last of which has the wrapped counter

curl -XPUT -H 'Content-Type: application/json' 'http://localhost:9200/bucket_agg_npe' -d '
{
  "mappings": {
    "doc": {
      "properties": {
        "bytes_in": { "type": "double"  },
        "@timestamp":  { "type":   "date"}
      }
    }
  }
}'

curl -XPOST -H 'Content-Type: application/json' 'http://localhost:9200/_bulk?pretty' -d '
{ "index" : { "_index" : "bucket_agg_npe", "_type" : "doc", "_id" : "1" } }
{ "bytes_in" : 100, "@timestamp" : "2018-08-10T00:00:00" }
{ "index" : { "_index" : "bucket_agg_npe", "_type" : "doc", "_id" : "2" } }
{ "bytes_in" : 200, "@timestamp" : "2018-08-10T00:05:00" }
{ "index" : { "_index" : "bucket_agg_npe", "_type" : "doc", "_id" : "3" } }
{ "bytes_in" : 0, "@timestamp" : "2018-08-10T00:10:00" }
'

Then query

curl -XGET -H 'Content-Type: application/json' 'http://localhost:9200/bucket_agg_npe/_search?size=0&pretty' -d '
{
  "aggs": {
    "histogram_buckets": {
      "date_histogram": {
        "field": "@timestamp",
        "time_zone": "UTC",
        "interval": "5m",
        "offset": 0,
        "order": {
          "_key": "asc"
        },
        "keyed": false,
        "min_doc_count": 0
      },
      "aggregations": {
        "@timestamp": {
          "max": {
            "field": "@timestamp"
          }
        },
        "bytes_in_avg": {
          "avg": {
            "field": "bytes_in"
          }
        },
        "bytes_in_derivative": {
          "derivative": {
            "buckets_path": [
              "bytes_in_avg"
            ],
            "gap_policy": "skip"
          }
        },
        "non_negative_bytes": {
          "bucket_script": {
            "buckets_path": {
              "bytes": "bytes_in_derivative"
            },
            "script": {
              "source": "params.bytes >= 0 ? params.bytes : null",
              "lang": "painless"
            },
            "gap_policy": "skip"
          }
        }
      }
    }
  }
}'

Returns

{
  "error" : {
    "root_cause" : [ ],
    "type" : "search_phase_execution_exception",
    "reason" : "",
    "phase" : "fetch",
    "grouped" : true,
    "failed_shards" : [ ],
    "caused_by" : {
      "type" : "script_exception",
      "reason" : "runtime error",
      "script_stack" : [
        "params.bytes >= 0 ? params.bytes : null",
        "                                   ^---- HERE"
      ],
      "script" : "params.bytes >= 0 ? params.bytes : null",
      "lang" : "painless",
      "caused_by" : {
        "type" : "null_pointer_exception",
        "reason" : null
      }
    }
  },
  "status" : 503
}

And the relevant log

[r.suppressed             ] [node-0] path: /bucket_agg_npe/_search, params: {pretty=, size=0, index=bucket_agg_npe}
 org.elasticsearch.action.search.SearchPhaseExecutionException: 
         at org.elasticsearch.action.search.AbstractSearchAsyncAction.onPhaseFailure(AbstractSearchAsyncAction.java:293) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.FetchSearchPhase$1.onFailure(FetchSearchPhase.java:91) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.onFailure(ThreadContext.java:708) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:39) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.common.util.concurrent.TimedRunnable.doRun(TimedRunnable.java:41) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37) [elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1135) [?:?]
         at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) [?:?]
         at java.lang.Thread.run(Thread.java:844) [?:?]
 Caused by: org.elasticsearch.script.ScriptException: runtime error
         at org.elasticsearch.painless.PainlessScript.convertToScriptException(PainlessScript.java:94) ~[?:?]
         at org.elasticsearch.painless.PainlessScript$Script.execute(params.bytes >= 0 ? params.bytes : null:1) ~[?:?]
         at org.elasticsearch.search.aggregations.pipeline.bucketscript.BucketScriptPipelineAggregator.reduce(BucketScriptPipelineAggregator.java:113) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.search.aggregations.InternalAggregation.reduce(InternalAggregation.java:138) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.search.aggregations.InternalAggregations.reduce(InternalAggregations.java:77) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.SearchPhaseController.reduceAggs(SearchPhaseController.java:525) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.SearchPhaseController.reducedQueryPhase(SearchPhaseController.java:502) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.SearchPhaseController.reducedQueryPhase(SearchPhaseController.java:419) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.SearchPhaseController$1.reduce(SearchPhaseController.java:738) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.FetchSearchPhase.innerRun(FetchSearchPhase.java:101) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.FetchSearchPhase.access$000(FetchSearchPhase.java:44) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.FetchSearchPhase$1.doRun(FetchSearchPhase.java:86) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:723) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         ... 5 more
 Caused by: java.lang.NullPointerException
         at org.elasticsearch.painless.Def.DefTodoubleImplicit(Def.java:705) ~[?:?]
         at org.elasticsearch.painless.PainlessScript$Script.execute(params.bytes >= 0 ? params.bytes : null:36) ~[?:?]
         at org.elasticsearch.search.aggregations.pipeline.bucketscript.BucketScriptPipelineAggregator.reduce(BucketScriptPipelineAggregator.java:113) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.search.aggregations.InternalAggregation.reduce(InternalAggregation.java:138) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.search.aggregations.InternalAggregations.reduce(InternalAggregations.java:77) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.SearchPhaseController.reduceAggs(SearchPhaseController.java:525) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.SearchPhaseController.reducedQueryPhase(SearchPhaseController.java:502) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.SearchPhaseController.reducedQueryPhase(SearchPhaseController.java:419) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.SearchPhaseController$1.reduce(SearchPhaseController.java:738) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.FetchSearchPhase.innerRun(FetchSearchPhase.java:101) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.FetchSearchPhase.access$000(FetchSearchPhase.java:44) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.action.search.FetchSearchPhase$1.doRun(FetchSearchPhase.java:86) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:723) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37) ~[elasticsearch-7.0.0-alpha1-SNAPSHOT.jar:7.0.0-alpha1-SNAPSHOT]
         ... 5 more

[elasticmachine]

Pinging @elastic/es-search-aggs

[rjernst]

I'm not sure the fact you could return null was ever documented, but it seems likely something that broke as the result of some recent refactoring in #32068. @original-brownbear can you take a look?

[original-brownbear]

@rjernst sure on it shortly :)

[original-brownbear]

@rjernst hmm supporting this kinda messes with the plan of improving performance by returning double directly from the Painless script. This wasn't an issue before since we returned Object from the script and then did a null check here https://github.com/elastic/elasticsearch/pull/32068/files#diff-c94184ea4ef180f10817aa2bbd41a8edL116.

I guess if we want to continue supporting this we will have to go back (a little) to returning Double and adding back the null check?

[colings86]

I think we should return Double and do the null check here. This is for 2 reasons:

1. Some calculations that are done with the bucket_script aggregation may not want to return a value for every bucket.
2. Not allowing null to be returned is a breaking change to at the very least we need to have deprecation warnings in 6.x and a path for migration of user's requests to account for the upcoming breaking change.

[davidkyle]

This is a ml/beats use case where the query is used in a template job configuration. Not allowing null is a breaking change for anyone running those ml jobs.

[original-brownbear]

How about: I'll make it return Double for now then and bring the null check back to fix things and we'll see if we can move to double in a reasonable way at some later point then?

[original-brownbear]

Fix incoming in #32811