[2.4] revert [SPARK-26021][SQL] replace minus zero with zero in Platform.putDouble/Float

[cloud-fan]

What changes were proposed in this pull request?

This PR reverts #23043 and its followup #23265, from branch 2.4, because it has behavior changes.

How was this patch tested?

existing tests

[cloud-fan]

cc @adoron @dongjoon-hyun @viirya

[viirya]

Agreed that we shouldn't make behavior change in 2.4.

[SparkQA]

Test build #100467 has finished for PR 23389 at commit 9be77a8.

• This patch fails due to an unknown error code, -9.
• This patch merges cleanly.
• This patch adds no public classes.

[viirya]

retest this please.

[SparkQA]

Test build #100469 has finished for PR 23389 at commit 9be77a8.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[dongjoon-hyun]

+1, LGTM.
This is a clean revert of SPARK-26021 (two patches).
Merged to branch-2.4.

[srowen]

On the general logic: if this is a correctness issue, then to fix it, you have to change incorrect behavior. Why would it be better to leave the correctness issue in 2.4? this is, further, a second behavior change then. Is there more context?

[srowen]

@cloud-fan @dongjoon-hyun given your email thread, I'm still not clear on the logic for reverting this one. Behavior changes are needed to fix correctness problems in behavior, no? and this has introduced two behavior changes in 2.4 now. I also just don't see any comments anywhere about the reasoning, but maybe I never found the right PR.

Is this also something we need to discuss?

[dongjoon-hyun]

Thanks, @srowen .

In the email thread, I don't make a judge on any of the cases. I collected and share our behavior to make a more clear communication way to encourage this kind of discussions. :)

given your email thread, I'm still not clear on the logic for reverting this one.

For the following question, #23043 (comment) was @cloud-fan 's decision comment on the original PR. That was the only one I found.

I also just don't see any comments anywhere about the reasoning

I feel in the same way with you, but I also remember that we made a different decision in this type of issues. For me, this has been a political decision instead of a technical decision. And, if there was a reverting request on the old branch, we tend to accept it because we are conservative and afraid of any unknown regression and pipeline failures.

Behavior changes are needed to fix correctness problems in behavior, no?

[srowen]

Right, worth discussing. Everything is a judgment call, but I'd like to understand the 'judgment'. The logic can't be: "we can't fix correctness issues because it means changing incorrect behavior to correct behavior". I assume the logic was: the behavior change causes more problems than fixing the correctness, which could be fine.

My particular issue here is that this PR changed a few things, and I infer that one thing was an issue (NaN handling?) when the important correctness issue was elsewhere (0.0 vs -0.0). Wholesale reverting might be the wrong call.

[cloud-fan]

It's a tradeoff between how serious is the bug and how risky is the patch. 0.0 vs -0.0 seems a minor bug (I don't know how this bug can cause serious problems in real-world queries) and the patch is non-trivial.