MDEV-38670 Unary minus on empty string returns -0

[3titoo]

Normalize -0.0 to +0.0 in Item_func_neg::real_op() to match binary subtraction behavior. Add a test case in type_newdecimal.

[CLAassistant]

All committers have signed the CLA.

[gkodinov]

This is a preliminary review.
Thank you for working on this!
You have pinpointed the problem correctly. But it needs more work to arrive at a complete solution, adequate testing and a solid commit message.

[3titoo]

key changes:

• I have added a utility in item_func.h (normalize_signed_zero(..))
• updated every real_op function in Item_func.cc to return normalize_signed_zero(res) which prevent -0.0
• updated every real_op functions in item_cmpfunc.cc to return normalize_signed_zero(res)
• added test cases to test every function has been updated

The behavior before is like :

The behavior now is like that:

[gkodinov]

Thanks for willing to make this a good and well-rounded database feature. Some further review comments below.

[3titoo]

Added classification and normalization criteria to the JIRA.
MDEV-38670

[3titoo]

i have edited Item *Item_float::neg(THD *thd)
because these tests:

CREATE TABLE t_signed_zero (a DOUBLE);
INSERT INTO t_signed_zero VALUES (-0e1),(0e1);
SELECT COUNT(DISTINCT a) FROM t_signed_zero;
SELECT a,COUNT(*) FROM t_signed_zero GROUP BY a;

It prints -0 and 0 as distinct numbers while they are equal
now after edits, it prints only 0 as a distinct number

[gkodinov]

Thanks for working on this. One small nitpick item below.
Please wait for the final review.

[gkodinov]

Do you really need to define this twice? item_func.cc already includes item.h (indirectly, but still).

[gkodinov]

Forgot to mention: please re-base on 10.6. This is safe to do and is a bug fix. So let's please assign the proper target version.

[vuvova]

Wouldn't it be enough to fix it in dtoa? So that it wouldn't print - before 0 ?

[3titoo]

Wouldn't it be enough to fix it in dtoa? So that it wouldn't print - before 0?

Fixing this in dtoa would only hide the sign during printing.

The problem is that the server internally treats -0 and +0 differently in some execution paths (e.g., grouping), even though they are numerically equal.
So the fix is applied where values are produced (unary minus, arithmetic functions) to canonicalize the value itself

[abarkov]

Wouldn't it be enough to fix it in dtoa? So that it wouldn't print - before 0?

Fixing this in dtoa would only hide the sign during printing.

The problem is that the server internally treats -0 and +0 differently in some execution paths (e.g., grouping), even though they are numerically equal. So the fix is applied where values are produced (unary minus, arithmetic functions) to canonicalize the value itself

We discussed this issue with the team and think that double values of 0.0 and -0.0 compare as equal. Your patch also relies on this fact by the way:

return value == 0.0 ? 0.0 : value;

Can you please give examples when grouping or some other execution paths give wrong results?

[3titoo]

Wouldn't it be enough to fix it in dtoa? So that it wouldn't print - before 0?

Fixing this in dtoa would only hide the sign during printing.
The problem is that the server internally treats -0 and +0 differently in some execution paths (e.g., grouping), even though they are numerically equal. So the fix is applied where values are produced (unary minus, arithmetic functions) to canonicalize the value itself

We discussed this issue with the team and think that double values of 0.0 and -0.0 compare as equal. Your patch also relies on this fact by the way:

return value == 0.0 ? 0.0 : value;

Can you please give examples when grouping or some other execution paths give wrong results?

This should print 1 when they are the same value, and group by should print only 0, not 0 and -0

But now, after edits:

[gkodinov]

LGTM from a preliminary review standpoint. Please take the alternative approach discussion with the final reviewer (Alexander).

[3titoo]

LGTM from a preliminary review standpoint. Please take the alternative approach discussion with the final reviewer (Alexander).

Thanks for the review!

@abarkov — since this involves choosing the overall approach for handling signed zero,
would you prefer normalizing it at the producer level (real_op return sites) or handling it elsewhere in the engine?

I followed approach 2 for consistency.