Avg aggregation over 0 documents/values yields Double.NaN if using transport client, but Double.POSITIVE_INFINITY if using REST client

[ahubmann]

Describe the feature: Running an avg aggregation when 0 documents/values match yields different values depending on connection type (TransportClient vs RestClient).

Elasticsearch version (bin/elasticsearch --version): 7.2.0 (and 6.x as well)

Plugins installed: []

JVM version (java -version): 11.0.3

OS version (uname -a if on a Unix-like system): Linux sascha 4.15.0-54-generic #58-Ubuntu SMP Mon Jun 24 10:55:24 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux

Description of the problem including expected versus actual behavior: Calculation of avg metric over 0 documents yields NaN if connected to Elasticsearch using transport client, but Double.POSITIVE_INFINITY if using REST client. IMHO the result of an operation should not depend on the connection type to the Elasticsearch cluster.

This seems to be related to the fact that internally, Elasticsearch takes two different routes to create the result of an avg aggregation:

• When using the TransportClient, this is done in InternalAvg as sum / count (https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalAvg.java#L69), where in case of count == 0 Java returns Double.NaN

• Whereas when using REST, this is done in ParsedAvg, explicitly setting Double.POSITIVE_INFINITY as the default (https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ParsedAvg.java#L55)

Steps to reproduce: (see also attached elastic-avg.zip)

1. Run Elasticsearch via Docker as run -it --rm -p 127.0.0.1:9200:9200 -p 127.0.0.1:9300:9300 -e "discovery.type=single-node" -e "cluster.name=elasticsearch" docker.elastic.co/elasticsearch/elasticsearch:7.2.0

2. Index a sample document:

IndexRequest indexRequest = new IndexRequest("test").id("1").source(XContentFactory.jsonBuilder().startObject().field("value", 1).endObject());
client.index(indexRequest, RequestOptions.DEFAULT);

3. Aggregate over a field with count 0:

protected static final AvgAggregationBuilder aggregation = AggregationBuilders.avg("test").field("value2");

4. Run aggregation using TransportClient:

// execute aggregation over transport client
SearchResponse response = client.prepareSearch().addAggregation(aggregation).execute().actionGet();
Avg result = response.getAggregations().get("test");

// for transport client, the result is NaN
assertTrue(Double.isNaN(result.value()));

5. Run aggregation using RestClient:

// execute aggregation over rest client
SearchRequest searchRequest = new SearchRequest();
SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder();
searchRequest.source(searchSourceBuilder);
searchSourceBuilder.aggregation(aggregation);
SearchResponse response = client.search(searchRequest, RequestOptions.DEFAULT);
Avg result = response.getAggregations().get("test");

// for rest client, the result is positive infinity
assertTrue(Double.isInfinite(result.value()));

Provide logs (if relevant):

[elasticmachine]

Pinging @elastic/es-core-features

[SivagurunathanV]

I would like to work on this issue.
@ahubmann @markharwood @hub-cap what is expected, in both the cases we need to return Double.POSITIVE_INFINITY?

[hub-cap]

Our general consensus is to replicate what the transport client returns. But I dont know these classes well enough to know what the "correct" answer is here. I will notify some team members to see if they can answer.

[hub-cap]

based on #24162, it would probably make sense for @cbuescher to comment on what the value should be, and whether he intended on the difference here.

[9deniz]

@hub-cap does it make sense to use TransportClient as a reference taking into account that

Deprecated in 7.0.0. The TransportClient is deprecated in favour of the Java High Level REST Client and will be removed in Elasticsearch 8.0.

In master branch there is no such class as TransportClient already.

[cbuescher]

based on #24162, it would probably make sense for @cbuescher to comment on what the value should be, and whether he intended on the difference here.

The problem in replicating the exact NaN/+/-Inf values as in the transport client the we encountered when writing the new REST client classes was that on the client side we can only use what we can parse from the REST response. In case of the Avg agg this e.g. means we no longer know what sum and count where on the server side, since we don't write those values to the response (XContent, most of the time JSON), but only the calculated value. This works for finite values but not for NaN/+/-Inf where we loose that information "over the wire", so we needed to pick one on the receiving side.
I don't see a simple way to fix this that doesn't involve changing the xContent serialization which comes with a lot of backward compatibility complications. As far as I remember there were also issues around our different xContent formats (Json, CBOR etc...) not handling NaN/+-Inf the same way across the board. I will see if I can dig up more info around that point.
I agree it would be nice to have the exact same behaviour in transport and rest client (which we generally aimed for), but I see no quick and easy fix here. I'd be interested in which cases NaN/+-Inf need to be distiguishable on the client side though.

[hub-cap]

@9deniz you are correct! This effort we did to get the HLRC up to speed was for 7, which is where the transport client code still lives. We tried to reach API parity w/ the transport client so users could migrate and not see weird things.

That being said, we can change APIs in 8 to what we want, its just a breaking (potentially) change. But I still think that it is worth reiterating what @cbuescher mentioned which is to know what the difference is between NaN/+-inf values, to know if it matters if we return a different value here in the HLRC.

[hub-cap]

Im closing this issue as something we wont fix. Simply knowing that the value is not real in any way should be sufficient, regardless of the NAN/POSITIVE_INFINTY status.