-
-
Notifications
You must be signed in to change notification settings - Fork 2k
MDEV-38670 Unary minus on empty string returns -0 #4632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Normalize -0.0 to +0.0 in Item_func_neg::real_op() to match binary subtraction behavior. Add test case in type_newdecimal. add result for tests fix tests Revert whitespace changes in type_newdecimal.result Revert whitespace changes in type_newdecimal.test
| if (res == 0.0) | ||
| res= 0.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To a human, this looks like something a compiler might consider no-op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… but, checking on https://godbolt.org/, it’s not the case for either GCC -O3 or Clang -O3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really don't know why it works, but when I remove the condition, it prints -0 when run ( select -''; ), which is the issue
gkodinov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| @@ -1825,7 +1825,11 @@ double Item_func_neg::real_op() | |||
| { | |||
| double value= args[0]->val_real(); | |||
| null_value= args[0]->null_value; | |||
| return -value; | |||
| double res= -value; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very good start. But it's definitely not all there is to it.
This will only cover the negation function which you happen to be using.
The problem is more wide-spread. It's everywhere a FP literal is used.
E.g. try "select 0e0 * -1e0".
I believe this is happening because MySQL does IEEE 754 arithmetic. And doesn't account for "the signed zero" (https://en.wikipedia.org/wiki/Signed_zero) phenomenon while doing it.
As a result many operations done on the real and double data types internally can result in a signed zero.
The question now is where do you want to stop with the signed zero.
Approach 1: You could just acknowledge its presence in the docs. I.e. fully accept it.
Approach 2: dispose of it in the producer. Audit all implementations of real_op() in Item subclasses and call a utility function for their return value to turn the signed zero into an unsigned zero when detected.
Approach 3: dispose of it in the consumers. Every place where real_op() is called call a utility function to turn the signed zero into an unsigned zero when detected.
Which one to take is a question for the final review of course.
But at this point (before we start wasting domain expert's time) I'd like to request that you have a good story that covers more than just negation. And the story needs to be recorded into either the jira or the commit message itself.
| @@ -1858,6 +1862,9 @@ my_decimal *Item_func_neg::decimal_op(my_decimal *decimal_value) | |||
| { | |||
| my_decimal2decimal(value.ptr(), decimal_value); | |||
| my_decimal_neg(decimal_value); | |||
| // Normalize -0 to +0 in decimal representation | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this? Do you actually have a test that hits this? I would be extremely surprised if you do.
| --echo # | ||
| SELECT @x := -''; | ||
| SELECT @y := 0 - ''; | ||
| SELECT -0.0, -'0', -' ', -@x, --@x; No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, these are not floating point literals. The tests with these are irrelevant.
On how to specify constants in MariaDB, see:
MariaDB [test]> create table t1 as select 0 as a, 0.0 as b, 0e0 as c;
Query OK, 1 row affected (0.042 sec)
Records: 1 Duplicates: 0 Warnings: 0
MariaDB [test]> show create table t1;
+-------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table |
+-------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| t1 | CREATE TABLE `t1` (
`a` int(1) NOT NULL,
`b` decimal(2,1) NOT NULL,
`c` double NOT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_uca1400_ai_ci |
+-------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.001 sec)
Please add relevant tests that cover all of your changes and nothing but.
| -1 | ||
| set sql_mode=default; | ||
| # End of 10.5 tests | ||
| # MDEV-38670 Unary minus on empty string should return 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func_math is failing on most of the build-bot platforms. Please fix this.
| @@ -1998,3 +1998,10 @@ select cast(1 as unsigned) - cast(2 as unsigned); | |||
| set sql_mode=default; | |||
|
|
|||
| --echo # End of 10.5 tests | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is customary to end a test file with a version test end echo command. Please add one for the target version: 13 in your case I believe, unless the final reviewer requests otherwise.
Normalize -0.0 to +0.0 in Item_func_neg::real_op() to match binary subtraction behavior. Add a test case in type_newdecimal.