Make the combine_script and reduce_script required in the scripted_metric aggregation

[colings86]

Currently int he scripted_metric aggregation only the map_script script is required. This can lead to users getting into problems such as [1] where they forget to, or don't realise they need to, provide a combine_script and/or reduce_script causing weird looking results in the response. I can't think of a reason why you would not want a combine_Script and/or reduce_script so I think we should make both of these mandatory. If someone does have a use-case where they don't need one of these scripts, it should be trivial to write a script that passes the input state straight to the return value.

I think the init_script should remain optional however, since I can see use cases where no set up is required before collecting documents.

In terms of bwc I think we will just need to add a deprecation warning if the scripted_metric agg is supplied without a combine_script and/or reduce_script warning the user that these scripts will be required in the next major version.

[1] https://discuss.elastic.co/t/scripted-metric-aggregations-sorting/143719/4

[elasticmachine]

Pinging @elastic/es-search-aggs

[albendz]

I'm wondering if I can pick this up.

Also, is there a way for us to provide a trivial script that they can reference in this call? It would just be a way to reduce error for users if they try and fail to write a trivial script by providing them with one.

[colings86]

@albendz If you'd like to work on this that would be great

We could potentially provide a trivial script int eh documentation but honestly I'm not sure its worth it as the output of the aggregation is not going to be very useful if the user doesn't provide combine and reduce scripts that are specific to their use case.

[albendz]

I can see that these scripts are used here: https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregationBuilder.java

I'm having trouble finding where the entry point to the API is defined. My IntelliJ config is a little messed up with the nested projects and while I work that out, a pointer to the API spec would be appreciated.

[colings86]

@albendz It should be a case of adding some checks in ScriptedMetricAggregationBuilder.doBuild so an exception is thrown if we try to build without a combine or reduce script provided in the builder.

I'm having trouble finding where the entry point to the API is defined

The Aggregation builders are the closest thing to an entry point that exists for aggregations since they are one of many components in the Search API. The entry point of the Search API itself can be found in RestSearchAction which will eventually call TransportSearchAction which is what actually start execution of the search request.

a pointer to the API spec would be appreciated.

We have a spec for the REST API which is used by the language clients but it current does not define the body of the request, only the HTTP method(s), URL paths, and URL parameters so I'm not sure you will find it useful for this task.

[albendz]

Thank you for those pointers. I will focus on adding validation in the builder.

[albendz]

Another question: should it be combine AND reduce scripts, combine OR reduce scripts, or combine XOR reduce scripts?

[colings86]

We should throw an exception if there is no reduce scripts. We should also throw an exception if there is no combine script. I think these two checks can be separate checks though, i.e.:

if (no combine_script) {
    throw exception
}
if (no reduce_script) {
    throw exception
}