expression: check Overflow int128 #4762

fzhedu · 2022-04-25T15:02:09Z

What problem does this PR solve?

Issue Number: close #4512

Problem Summary:

What is changed and how it works?

Check List

Tests

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

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2022-04-25T15:02:10Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

LittleFall
yibin87

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

fuzhe1989 · 2022-04-26T00:44:31Z

consider port https://github.com/ClickHouse/ClickHouse/blob/master/base/base/arithmeticOverflow.h to TiFlash?

yibin87

It'll be nice to explain why muloverflow affects comparasion result.

fzhedu · 2022-05-12T14:11:05Z

consider port https://github.com/ClickHouse/ClickHouse/blob/master/base/base/arithmeticOverflow.h to TiFlash?

https://github.com/ClickHouse/ClickHouse/blob/master/base/base/arithmeticOverflow.h#L179
/// Overflow check is not implemented for big integers.

but here needs to checkout overflow.

fzhedu · 2022-05-12T14:11:48Z

It'll be nice to explain why muloverflow affects comparasion result.

a git positive value becomes negective, so the compare resultes are wrong.

yibin87

LGTM

windtalker · 2022-05-13T00:52:53Z

libs/libcommon/include/common/arithmeticOverflow.h

-    unsigned __int128 a = (x > 0) ? x : -x;
-    unsigned __int128 b = (y > 0) ? y : -y;
-    return (a * b) / b != a;
+    return res/x != y; /// whether overflow int128


How about add some unit test for this overflow check?

fzhedu · 2022-05-13T05:04:26Z

libs/libcommon/src/tests/gtest_arithmetic_overflow.cpp

+    /// 2^254 << 1 = 2^255
+    is_overflow = common::mulOverflow(int_254, Int256(2), res256);
+    ASSERT_EQ(is_overflow, true);


@LittleFall is this as expected?

I think it is expected, the max limit of signed Int256 is (1<<255)-1

According to the boost doc and implement, the Int256 in boost includes 256bit data and 1bit sign, so the max/min value is different from our expectation.

https://www.boost.org/doc/libs/1_60_0/libs/multiprecision/doc/html/boost_multiprecision/tut/ints/cpp_int.html

after discussion, this won't affect our current logic. the unexpected common::mulOverflow only has one caller https://github.com/pingcap/tiflash/blob/master/dbms/src/Core/DecimalComparison.h#L153-L169, and it won't break too.

by the way, we need some comments on this function.

windtalker · 2022-05-16T08:20:32Z

libs/libcommon/include/common/arithmeticOverflow.h

@@ -129,6 +127,6 @@ inline bool mulOverflow(Int256 x, Int256 y, Int256 & res)
    {
        return true;
    }
-    return false;
+    return res >= Int256((Int256(0x1) << 255)); /// whether overflow int256


Why need add this?

Why need add this?

please see https://github.com/pingcap/tiflash/pull/4762/files#r873432216

Signed-off-by: fzhedu <fzhedu@gmail.com>

fzhedu · 2022-05-20T02:55:00Z

/merge

ti-chi-bot · 2022-05-20T02:55:01Z

@fzhedu: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-05-20T02:55:03Z

This pull request has been accepted and is ready to merge.

Commit hash: d688bb8

sre-bot · 2022-05-20T03:10:40Z

Coverage for changed files

Filename                                                                                                          Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/jenkins/agent/workspace/tiflash-build-common/tiflash/libs/libcommon/include/common/arithmeticOverflow.h          21                 9    57.14%          13                 9    30.77%          56                33    41.07%           4                 1    75.00%
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                                                                  21                 9    57.14%          13                 9    30.77%          56                33    41.07%           4                 1    75.00%

Coverage summary

Functions  MissedFunctions  Executed  Lines   MissedLines  Cover
18224      9787             46.30%    204122  97976        52.00%

full coverage report (for internal network access only)

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2022-05-20T03:23:16Z

In response to a cherrypick label: new pull request created: #4942.

ti-chi-bot · 2022-05-20T03:23:53Z

In response to a cherrypick label: new pull request created: #4943.

ti-chi-bot · 2022-05-20T03:24:30Z

In response to a cherrypick label: new pull request created: #4944.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2022-05-20T03:25:09Z

In response to a cherrypick label: new pull request created: #4945.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2022-05-20T03:25:47Z

In response to a cherrypick label: new pull request created: #4946.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2022-05-20T03:26:24Z

In response to a cherrypick label: new pull request created: #4947.

close #4512

fzhedu requested review from windtalker and LittleFall April 25, 2022 15:02

ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 26, 2022

yibin87 reviewed Apr 29, 2022

View reviewed changes

yibin87 approved these changes May 12, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 12, 2022

windtalker reviewed May 13, 2022

View reviewed changes

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 13, 2022

fzhedu commented May 13, 2022

View reviewed changes

windtalker reviewed May 16, 2022

View reviewed changes

LittleFall approved these changes May 16, 2022

View reviewed changes

ti-chi-bot removed the status/LGT1 Indicates that a PR has LGTM 1. label May 16, 2022

formated

d688bb8

Signed-off-by: fzhedu <fzhedu@gmail.com>

fzhedu force-pushed the overflowInt128 branch from 4463059 to d688bb8 Compare May 20, 2022 02:54

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label May 20, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 20, 2022

ti-chi-bot merged commit 71613fd into pingcap:master May 20, 2022

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request May 20, 2022

This is an automated cherry-pick of pingcap#4762

180ce11

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request May 20, 2022

expression: check Overflow int128 (#4762) #4942

Merged

12 tasks

ti-chi-bot mentioned this pull request May 20, 2022

expression: check Overflow int128 (#4762) #4943

Merged

12 tasks

ti-chi-bot mentioned this pull request May 20, 2022

expression: check Overflow int128 (#4762) #4944

Closed

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request May 20, 2022

This is an automated cherry-pick of pingcap#4762

cfc3f3f

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request May 20, 2022

expression: check Overflow int128 (#4762) #4945

Merged

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request May 20, 2022

This is an automated cherry-pick of pingcap#4762

39f7f76

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request May 20, 2022

expression: check Overflow int128 (#4762) #4946

Merged

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request May 20, 2022

This is an automated cherry-pick of pingcap#4762

f749611

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request May 20, 2022

expression: check Overflow int128 (#4762) #4947

Merged

12 tasks

ti-chi-bot added a commit that referenced this pull request Jun 16, 2022

expression: check Overflow int128 (#4762) (#4943)

0ecacbe

close #4512

ti-chi-bot added a commit that referenced this pull request Jun 16, 2022

expression: check Overflow int128 (#4762) (#4947)

0c77d5d

close #4512

ti-chi-bot added a commit that referenced this pull request Jun 16, 2022

expression: check Overflow int128 (#4762) (#4942)

b67ae08

close #4512

ti-chi-bot added a commit that referenced this pull request Jun 16, 2022

expression: check Overflow int128 (#4762) (#4945)

6ca9ad8

close #4512

ti-chi-bot added a commit that referenced this pull request Jun 17, 2022

expression: check Overflow int128 (#4762) (#4946)

92f6e31

close #4512

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: check Overflow int128 #4762

expression: check Overflow int128 #4762

fzhedu commented Apr 25, 2022

ti-chi-bot commented Apr 25, 2022 •

edited

Loading

fuzhe1989 commented Apr 26, 2022

yibin87 left a comment

fzhedu commented May 12, 2022

fzhedu commented May 12, 2022

yibin87 left a comment

windtalker May 13, 2022

fzhedu May 13, 2022

fzhedu May 13, 2022

LittleFall May 13, 2022

LittleFall May 16, 2022

LittleFall May 16, 2022 •

edited

Loading

windtalker May 16, 2022

fzhedu May 16, 2022

fzhedu commented May 20, 2022

ti-chi-bot commented May 20, 2022

ti-chi-bot commented May 20, 2022

sre-bot commented May 20, 2022

ti-chi-bot commented May 20, 2022

ti-chi-bot commented May 20, 2022

ti-chi-bot commented May 20, 2022

ti-chi-bot commented May 20, 2022

ti-chi-bot commented May 20, 2022

ti-chi-bot commented May 20, 2022

expression: check Overflow int128 #4762

expression: check Overflow int128 #4762

Conversation

fzhedu commented Apr 25, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 25, 2022 • edited Loading

fuzhe1989 commented Apr 26, 2022

yibin87 left a comment

Choose a reason for hiding this comment

fzhedu commented May 12, 2022

fzhedu commented May 12, 2022

yibin87 left a comment

Choose a reason for hiding this comment

windtalker May 13, 2022

Choose a reason for hiding this comment

fzhedu May 13, 2022

Choose a reason for hiding this comment

fzhedu May 13, 2022

Choose a reason for hiding this comment

LittleFall May 13, 2022

Choose a reason for hiding this comment

LittleFall May 16, 2022

Choose a reason for hiding this comment

LittleFall May 16, 2022 • edited Loading

Choose a reason for hiding this comment

windtalker May 16, 2022

Choose a reason for hiding this comment

fzhedu May 16, 2022

Choose a reason for hiding this comment

fzhedu commented May 20, 2022

ti-chi-bot commented May 20, 2022

ti-chi-bot commented May 20, 2022

sre-bot commented May 20, 2022

Coverage for changed files

Coverage summary

ti-chi-bot commented May 20, 2022

ti-chi-bot commented May 20, 2022

ti-chi-bot commented May 20, 2022

ti-chi-bot commented May 20, 2022

ti-chi-bot commented May 20, 2022

ti-chi-bot commented May 20, 2022

ti-chi-bot commented Apr 25, 2022 •

edited

Loading

LittleFall May 16, 2022 •

edited

Loading