NPE when using expression script aggregation in multi level aggregations

[sherry-ger]

Elasticsearch version (bin/elasticsearch --version):
5.5.3

Plugins installed: []
x-pack

Steps to reproduce:

1. Create an index using the following mappings.

{
  "testindex": {
    "mappings": {
      "mytype": {
        "properties": {
          "name": {
            "type": "text",
            "fields": {
              "raw": {
                "type": "keyword"
              }
            }
          },
          "other": {
            "type": "long"
          },
          "recid": {
            "type": "integer"
          },
          "rectype": {
            "type": "text",
            "fields": {
              "raw": {
                "type": "keyword"
              }
            }
          },
          "total": {
            "type": "long"
          },
          "used": {
            "type": "long"
          }
        }
      }
    }
  }
}

2. Insert an document.

POST testindex/mytype
{
  "recid": 1234,
  "total": 60,
  "rectype": "type1",
  "name": "John Smith",
  "used": 42,
  "other": 5
}

3. Perform the following aggregation query.

GET testindex/mytype/_search
{
  "size": 0,
  "aggregations": {
    "ByRecType": {
      "terms": {
        "field": "rectype.raw"
      },
      "aggregations": {
        "ByRecId": {
          "terms": {
            "field": "recid"
          },
          "aggregations": {
            "ByName": {
              "terms": {
                "field": "name.raw"
              },
              "aggregations": {
                "Sum": {
                  "sum": {
                    "script": {
                      "inline": "doc['used'].value + doc['total'].value + doc['other'].value",
                      "lang": "expression"
                    }
                  }
                }
              }
            }
          }
        }
      }
    }
  }
}

4. Observe the NPE error

{
  "error": {
    "root_cause": [
      {
        "type": "null_pointer_exception",
        "reason": null
      }
    ],
    "type": "search_phase_execution_exception",
    "reason": "all shards failed",
    "phase": "query",
    "grouped": true,
    "failed_shards": [
      {
        "shard": 0,
        "index": "testindex",
        "node": "ELj0Q7R_T_unF207CLwU7A",
        "reason": {
          "type": "null_pointer_exception",
          "reason": null
        }
      }
    ]
  },
  "status": 500
}

Note: If the script type were painless, the aggregation works fine.

Provide logs (if relevant):

npe.txt

[rjernst]

@sherry-ger This does not reproduce for me. I have tried with and without xpack, using your example requests, on 5.5, 5.6 and master. Is there something else about your setup that may be relevant?

[rjernst]

I was finally able to reproduce this, and it is due to a null Scorer being passed into painless from aggregations. @jpountz Can you or someone that knows aggs take a look? Also note this does not reproduce on 6.0 or master.

[martijnvg]

Thanks @rjernst for digging into this. I'll work on a fix.

[martijnvg]

This issue occurs only when terms aggs are executed in breath_first mode (which is default here). With depth_first no error occurs.

BestBucketsDeferringCollector does not create and delegate a scorer if the aggs are not using scores (BestBucketsDeferringCollector.java line 156 (5.6 branch)), which makes sense in this case. The problem is that in AggregatorFactory.java line 133, it just delegates a null scorer instance. I think that should be changed, if the scorer hasn't been set then the scorer shouldn't be delegated. Also it is just weird to supply a null scorer to LeafCollector#setScorer(...). The jdocs don't mention what happens if someone does this, but I think many implementation will fail if they were supplied with a null scorer. @colings86 What do you think?

Although the bug doesn't manifest in 6.0 and later, I think we should apply this fix on master, 6.x and 6.0 branches too. ExpressionSearchScript (and other impls) have been changed to be lenient with a null scorer. There is a TODO about this in SearchScript#getScore() method.

[colings86]

@martijnvg I agree with all your points, we should not be trying to pass down a scorer if no scorer has been set. I also agree that the fix should be applied to 6.0 6.x and master branches too.

[martijnvg]

Thanks @colings86. I'll open a PR.

[jpountz]

Thanks @martijnvg !