Fix AvgTests error on -0.0 avg

[ivancea]

Fixes #113225

[elasticsearchmachine]

Pinging @elastic/es-analytical-engine (Team:Analytics)

[not-napoleon]

Maybe I'm reading this wrong, but it looks to me like this will accept -0.0 as the correct output. We really shouldn't be returning negative zeros from any operations. (In fact, probably AbstractAggregatorTestCase#extractResultsFromAggregator should assert that we never return a forbidden double value).

[ivancea]

Maybe I'm reading this wrong, but it looks to me like this will accept -0.0 as the correct output. We really shouldn't be returning negative zeros from any operations.

@not-napoleon TestCaseSupplier provides -0.0 values, so if it can be an input, it should also be an output.

In ES, I see this: https://www.elastic.co/guide/en/elasticsearch/reference/current/number.html#_which_type_should_i_use (Just above that anchor)

The double, float and half_float types consider that -0.0 and +0.0 are different values.

So if ES also allows it, I would allow it too. And for this case, the avg of -0.0 should be -0.0 too (IMO) to be as precise as possible.

I don't have an specific opinion on this topic, I'm just basing my thought process on what we currently have (And compatibility with ES)

[not-napoleon]

to be as precise as possible.

It's not more precise, it's just more confusing. The IEEE spec for floating points says that the two zeros should be considered equal when possible. Java honors this with it's binary comparison operators (== and friends), but not with Double.equals(). You can read the docs on the double class to get an understanding for why this is the case. Practically, this means that if we start allowing negative zeros in the language, we need to be VERY clear on where they compare via == and where they compare via Double.equals() or Number.equals(). It becomes extremely trappy for users if that behavior is not entirely consistent.

We made the decision early on to not allow -0.0, positive or negative infinity, or NaN as possible values in ESQL. We should be consistent with that here. /cc @costin

[not-napoleon]

so if it can be an input, it should also be an output.

It's an input on the test precisely to test that it isn't an output.

[nik9000]

We made the decision early on to not allow -0.0, positive or negative infinity, or NaN as possible values in ESQL. We should be consistent with that here. /cc @costin

I think we should be consistent with this decision. Our functions should never receive any of those forbidden doubles. We could (should?) add an assertion to the generated code that reads doubles for EVAL and STATS that asserts that we don't receive those numbers. They should always come in as null. And they should always be emitted as null.

ES might support these but they are all null to ESQL. One day we may change this but that's a difficult thing to change at the moment.

[not-napoleon]

ES might support these but they are all null to ESQL. One day we may change this but that's a difficult thing to change at the moment.

Well, the infinities and NaN are null; -0.0 is 0.

[ivancea]

Ok, so:

• To ensure it's not an output, I'll add a check to tests like we have to NaN and infinities
• I'll revert the changes on AvgTests for now

Then, if we have to keep the -0.0 as a potential input in tests, but forbid it as an output, as many functions are currently returning it (Max, MvMax, Min, Values...), I'll fix them by doing a conversion (-0.0 -> 0.0).
I could add it in the agg/evaluator implementers, but it would affect even functions never emitting -0.0. So I'll see first how many functions are failing with this