Skip to content
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.

Sign up for GitHub

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

Jump to bottom

expression: change the round rule for approximate value to round to nearest even #21324

Merged
merged 2 commits into from
Dec 10, 2020
Merged

expression: change the round rule for approximate value to round to nearest even #21324

merged 2 commits into from
Dec 10, 2020

Conversation

xuhui-lu
Copy link
Contributor

@xuhui-lu xuhui-lu commented Nov 26, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number: close #1108

Problem Summary:
The Round rule for approximate-value numbers is “round half away from zero”, which is different from the “round to nearest even” rule used by MySQL.

What is changed and how it works?

What's Changed:
The round rule for approximate-value numbers is changed to “round to nearest even” rule.

How it Works:
select cast(25E-1 as signed)

+-----------------------+
| cast(25E-1 as signed) |
+-----------------------+
| 2 |
+-----------------------+

Related changes

  • Need to cherry-pick to the release branch

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Side effects

  • Performance regression
    • Consumes more CPU
    • Consumes more MEM
  • Breaking backward compatibility

Release note

  • Use “round to nearest even” rule instead of “round half away from zero” for approximate-value numbers

@xuhui-lu xuhui-lu requested a review from a team as a code owner November 26, 2020 08:49
@xuhui-lu xuhui-lu requested review from wshwsh12 and removed request for a team November 26, 2020 08:49
@ti-srebot ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Nov 26, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Nov 26, 2020

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

@xuhui-lu xuhui-lu force-pushed the xlu/round-to-even branch 6 times, most recently from 2e9197f to 2c77f9d Compare November 26, 2020 09:59
@ichn-hu ichn-hu mentioned this pull request Nov 26, 2020
Closed
@xuhui-lu
Copy link
Contributor Author

xuhui-lu commented Dec 1, 2020

@ichn-hu hello hu, may I get someone's review for this PR?

@ichn-hu
Copy link
Contributor

ichn-hu commented Dec 1, 2020

@ichn-hu hello hu, may I get someone's review for this PR?

Sure, you can /cc @the-name-of-reviewer to ask for review.

@ichn-hu
Copy link
Contributor

ichn-hu commented Dec 1, 2020

/cc @ichn-hu

ichn-hu
ichn-hu requested changes Dec 1, 2020
View reviewed changes
expression/builtin_time_test.go
@@ -2155,7 +2155,7 @@ func (s *testEvaluatorSuite) TestMakeTime(c *C) {

{[]interface{}{0, 58.4, 0}, "00:58:00"},
{[]interface{}{0, "58.4", 0}, "00:58:00"},
{[]interface{}{0, 58.5, 1}, "00:59:01"},
{[]interface{}{0, 58.5, 1}, "00:58:01"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case can't be changed, because in MySQL:

MySQL [test]> select maketime(0, 58.5, 1);
+----------------------+
| maketime(0, 58.5, 1) |
+----------------------+
| 00:59:01             |
+----------------------+
1 row in set (0.000 sec)

I remembered that for time, it is rounded up, so you might want to use the original version for time.

Copy link
Contributor Author

@xuhui-lu xuhui-lu Dec 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not exactly, I think in your case, 58.5 is treated as a decimal type, and in the unit test, it will be treated as type float. e.g.
MySQL [test]> select maketime(0, 585E-1, 1);
+----------------------+
| maketime(0, 585E-1, 1) |
+----------------------+
| 00:58:01 |
+----------------------+
1 row in set (0.000 sec)

@xuhui-lu xuhui-lu force-pushed the xlu/round-to-even branch 10 times, most recently from a2e90ab to 1c89928 Compare December 7, 2020 10:22
@xuhui-lu xuhui-lu requested a review from ichn-hu December 7, 2020 10:22
wshwsh12
wshwsh12 reviewed Dec 8, 2020
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the case to unit test.

select cast(25E-1 as signed)
+-----------------------+
| cast(25E-1 as signed) |
+-----------------------+
| 2 |
+-----------------------+

@ti-srebot
Copy link
Contributor

@xuhui-lu merge failed.

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Dec 9, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

1 similar comment
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@xuhui-lu merge failed.

@XuHuaiyu
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@xuhui-lu merge failed.

@wshwsh12
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@xuhui-lu merge failed.

@wshwsh12
Copy link
Contributor

/run-unit-test

@xuhui-lu
Copy link
Contributor Author

/run-unit-test

@wshwsh12
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@xuhui-lu
Copy link
Contributor Author

@wshwsh12 do I need to fix anything? Or the unit test failure is caused by flakey stuff?

I saw error like

[2020-12-10T03:16:23.880Z] FAIL: point_get_test.go:601: testPointGetSuite.TestPointGetReadLock
[2020-12-10T03:16:23.880Z] 
[2020-12-10T03:16:23.880Z] point_get_test.go:629:
[2020-12-10T03:16:23.880Z]     c.Assert(explain, Matches, ".*num_rpc.*")
[2020-12-10T03:16:23.880Z] ... value string = "[Point_Get_1 1.00 1 root table:point time:3.28µs, loops:2,  handle:1 N/A N/A]"
[2020-12-10T03:16:23.880Z] ... regex string = ".*num_rpc.*"

@wshwsh12
Copy link
Contributor

@xuhui-lu I think it is an unstable test, and it isn't related to this pr. We can merge this pr now...
The unstable test will be fixed in another pr.

@ti-srebot ti-srebot merged commit 8a3705e into pingcap:master Dec 10, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Dec 10, 2020
@ti-srebot
cherry pick pingcap#21324 to release-4.0
ba1f038 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Dec 10, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #21628

ti-srebot added a commit that referenced this pull request Jan 26, 2021
@ti-srebot
expression: change the round rule for approximate value to `round to …
89622d1 
…nearest even` (#21324) (#21628)

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@feitian124 feitian124 mentioned this pull request Aug 11, 2021
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjhuang2016 wjhuang2016 wjhuang2016 left review comments

@wshwsh12 wshwsh12 wshwsh12 approved these changes

@ti-srebot ti-srebot ti-srebot left review comments

@ichn-hu ichn-hu Awaiting requested review from ichn-hu

Assignees
No one assigned
Labels
component/expression first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Round result is different depending on whether its argument is exact or approximate
7 participants
@xuhui-lu @sre-bot @ichn-hu @wjhuang2016 @wshwsh12 @ti-srebot @XuHuaiyu