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.

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

Refactor decimal multiplication #4211

Merged
merged 1 commit into from
Mar 22, 2022

Conversation

satanson
Copy link
Contributor

@satanson satanson commented Mar 18, 2022

What type of PR is this:

  • bug
  • feature
  • enhancement
  • others

Which issues of this PR fixes :

Problem Summary(Required) :

Old decimal multiplication scale adjust rules is not robust, becase of a narrow decimal multiplied by another narrow decimal yields a narrow decimal, too. for example decimal64(10,2) * decimal64(10,2) yields decimal32(18,4), the result maybe overflow. so we must cast the narrow decimal to wide decimal if necessary to void overflow.

New decimal multiplication scale adjust rules as follows:
decimal(p1,s1) * decimal(p2, s2) => decimal(p, s)
At first, denote p' = p1 + p2, s' = s1+s2;

  1. in case of p' <= 38, result never overflows, use the narrowest decimal type that can holds the result. so:
    p = p1+ p2, s = s1+s2;
    for examples:
    decimal32(4,3) * decimal32(4,3) => decimal32(8,6);
    decimal64(15,3) * decimal32(9,4) => decimal128(24,7).
  2. in case of p' > 38 and s' <=38, the multiplication is computable but the result maybe overflow, so use decimal128 arithmetic and adopt maximum decimal precision(38) as precision of the result. so:
    p = 38, s = s1 + s2
    for examples:
    decimal128(23,5) * decimal64(18,4) => decimal128(38, 9).
  3. in case of s' > 38, the decimal is too large to hold in a decimal128, so report error. decimal's scale exceeds limit 38.

decimal arithmetic are testified both in disable/enable overflow check in BE, so now we turn on overflow check on BE for decimal arithmetic operations.

@satanson satanson force-pushed the refactor_decimal_multiply branch 4 times, most recently from feead53 to 902203b Compare March 18, 2022 04:32
@satanson
Copy link
Contributor Author

run starrocks_fe_unittest

@satanson
Copy link
Contributor Author

run starrocks_fe_unittest

murphyatwork
murphyatwork previously approved these changes Mar 18, 2022
@wanpengfei-git
Copy link
Collaborator

Can't set status; build failed.

@satanson
Copy link
Contributor Author

run starrocks_fe_unittest

1 similar comment
@satanson
Copy link
Contributor Author

run starrocks_fe_unittest

kangkaisen
kangkaisen previously approved these changes Mar 21, 2022
@satanson
Copy link
Contributor Author

run starrocks_be_unittest

@satanson
Copy link
Contributor Author

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Mar 21, 2022

rebase

✅ Branch has been successfully rebased

@wanpengfei-git wanpengfei-git force-pushed the refactor_decimal_multiply branch from 0c397e2 to 2c380c2 Compare March 21, 2022 12:47
@satanson
Copy link
Contributor Author

run starrocks_fe_unittest

@satanson
Copy link
Contributor Author

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2022

rebase

✅ Branch has been successfully rebased

@wanpengfei-git wanpengfei-git force-pushed the refactor_decimal_multiply branch from 2c380c2 to 2db3785 Compare March 22, 2022 03:03
@satanson
Copy link
Contributor Author

run starrocks_fe_unittest

@wanpengfei-git
Copy link
Collaborator

[FE PR Coverage check]

😍 pass : 21 / 21 (100.00%)

file detail

path covered line new line coverage
🔵 com/starrocks/analysis/ArithmeticExpr.java 21 21 100.00%

@stdpain stdpain merged commit 31d53c2 into StarRocks:main Mar 22, 2022
@satanson
Copy link
Contributor Author

@mergify backport branch-2.2

mergify bot pushed a commit that referenced this pull request Mar 22, 2022
(cherry picked from commit 31d53c2)

# Conflicts:
#	fe/fe-core/src/test/java/com/starrocks/sql/plan/AggregateTest.java
#	fe/fe-core/src/test/java/com/starrocks/sql/plan/ExpressionTest.java
@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2022

backport branch-2.2

✅ Backports have been created

Hey, I reacted but my real name is @Mergifyio

satanson added a commit to satanson/starrocks that referenced this pull request Mar 22, 2022
liuyehcf pushed a commit to liuyehcf/starrocks that referenced this pull request Apr 18, 2022
kangkaisen pushed a commit that referenced this pull request Apr 18, 2022
* Refactor decimal multiplication (#4211)

* Fixup: decimal * decimal(0,0) report error (#4359)

Co-authored-by: satanson <ranpanf@gmail.com>
liuyehcf pushed a commit to liuyehcf/starrocks that referenced this pull request Apr 18, 2022
satanson added a commit to satanson/starrocks that referenced this pull request Apr 18, 2022
* Refactor decimal multiplication (StarRocks#4211)

* Fixup: decimal * decimal(0,0) report error (StarRocks#4359)

Co-authored-by: satanson <ranpanf@gmail.com>
jaogoy pushed a commit to jaogoy/starrocks that referenced this pull request Nov 15, 2023
* update unload by flink connector doc

* Update Flink_connector.md

(cherry picked from commit 522585b)

# Conflicts:
#	unloading/Flink_connector.md

Co-authored-by: amber-create <57167462+amber-create@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants