[Scripting] BWC Incompatible Change for Bucket Aggregation Context

[jdconrad]

Recent changes to the bucket aggregation context are backwards incompatible for specific return cases in Painless (#32068). The expected return within this context changed from def (Object) to Double. This causes an issue with casting as it is illegal to cast from one boxed type to another. This means that scripts previously were allowed to do the following where value is a def representing double:

value > 0 ? value : 0

This previous script allowed the auto-return mechanism to cast 0 to Integer and return that since def was the expected return type. Now this will cause a ClassCastException because casting from Integer to Double is not allowed in Painless or Java (included as we follow a casting model very similar to Java's).

My recommended fix is to have this context return Object for now until we can come up with a better solution for deprecation. Note that if this context didn't require null, it would be okay to use double since it's legal to cast from int to double as unboxed types.

This started in version 6.5 and was caught by a Kibana script using a similar example to the one given.

ping @original-brownbear @rjernst

[elasticmachine]

Pinging @elastic/es-core-infra

[jakelandis]

Here is a reproduction case:

DELETE test
POST test/_doc/_bulk
{ "index" :{}}
{ "foo" : 1}
{ "index" :{}}
{ "foo" : 2 }
{ "index" :{}}
{ "foo" : 3 }

POST /test/_search
{
  "size": 0,
  "aggs": {
    "foo": {
      "date_histogram": {
        "field": "foo",
        "interval": "1ms"
      },
      "aggs": {
        "mymax": {
          "max": {
            "field": "foo"
          }
        },
        "mybucket_script": {
          "bucket_script": {
            "buckets_path": {
              "mymax": "mymax"
            },
            "script": "params.mymax > 0 ? params.mymax : 0"
          }
        }
      }
    }
  }
}

All is good... now introduce a 0

POST test/_doc
{
  "foo": 0
}

Re-run the search and

{
  "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.mymax > 0 ? params.mymax : 0",
        "^---- HERE"
      ],
      "script": "params.mymax > 0 ? params.mymax : 0",
      "lang": "painless",
      "caused_by": {
        "type": "class_cast_exception",
        "reason": "java.lang.Integer cannot be cast to java.lang.Double"
      }
    }
  },
  "status": 503
}

[jdconrad]

Briefly talked to @rjernst about this. I'm thinking over the behavior of return values in Painless. It's possible it makes more sense to make a change to casting model in the singular case because it will be better for the user.

[original-brownbear]

@jdconrad @rjernst

My recommended fix is to have this context return Object for now until we can come up with a better solution for deprecation.

I'd be in favour of this as a short-term fix. We did exactly this elsewhere too when trying to make the new contexts return a specific type and running into trouble.

[jdconrad]

Fixed by #35653