Fix AvgTests error on -0.0 avg

[ivancea]

Fixes #113225
Fixes #114175

[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

[ivancea]

@not-napoleon After some checks and talks about this:

• Added some "discoveries" about -0.0 here, to tackle whenever we want to work on this: [ES|QL] Don't return negative zero #104866 (comment)
• Restored this PR with the Avg fix. It does nothing around -0.0 directly, so I think it's safe to merge whatever the decision we take. Right now, it's agnostic.
  • I left the little refactor on tests: removing repeated code, and adding test cases of single values to aggs, as they are handled differently (This test was failing 1/1000 times because of this)

[not-napoleon]

Javadoc would be nice here.

[ivancea]

Added!

[not-napoleon]

This is fine for now.

[elasticsearchmachine]

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 113272