[improvement](decimal) use new way for decimal arithmetic precision promotion by jacktengg · Pull Request #27787 · apache/doris

jacktengg · 2023-11-30T01:41:27Z

Proposed changes

Issue Number: close #xxx

FE changes from #27492

decimalv3: use 38, min(decimalOverflowScale, ret_scale) as result precision if overflow
decimalv2 with largeint or bigint will promotion to decimalv3
decimalOverflowScale could be config by session variable

BE:

throw exception if it overflows for decimal arithmetics
throw exception if it overflows when casting among number types

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

github-actions

clang-tidy made some suggestions

yiguolei · 2023-11-30T01:58:37Z

 } while (0)
+
+#define THROW_ARITHMETIC_OVERFLOW_ERRROR \
+ throw Exception(ErrorCode::ARITHMETIC_OVERFLOW_ERRROR, "Arithmetic overflow")


this macro is not reasonable.
If we define such a macro, then we may need define many many macros... each error code need a macro

yiguolei · 2023-11-30T02:09:32Z


+template <typename T>
+constexpr vectorized::UInt32 get_number_max_digits() {
+ if constexpr (std::is_same_v<T, vectorized::UInt8> || std::is_same_v<T, vectorized::Int8>) {


add some comment

yiguolei · 2023-11-30T02:29:46Z

+ if constexpr (narrow_integral) {
+ LOG(WARNING) << "multiply narrow scale, negative, narrow integral";
+ if (UNLIKELY(converted_value.value < min_result.value)) {
+ THROW_ARITHMETIC_OVERFLOW_ERRROR;


throw exception is better than macro

jacktengg · 2023-11-30T16:07:04Z

run buildall

jacktengg · 2023-12-01T01:35:52Z

run buildall

jacktengg · 2023-12-03T15:18:52Z

run buildall

jacktengg · 2023-12-04T01:23:27Z

run pipelinex_p0

jacktengg · 2023-12-04T01:26:55Z

run buildall

jacktengg · 2023-12-04T06:51:18Z

run buildall

Gabriel39 · 2023-12-04T07:04:13Z

 return Status::Error<false>(e.code(), e.to_string()); \
 } \
- } while (0)
+ } while (0)


Add an empty line

jacktengg · 2023-12-04T07:13:13Z

run buildall

doris-robot · 2023-12-04T09:03:16Z

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 43.95 seconds
stream load tsv: 560 seconds loaded 74807831229 Bytes, about 127 MB/s
stream load json: 18 seconds loaded 2358488459 Bytes, about 124 MB/s
stream load orc: 64 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 34 seconds loaded 861443392 Bytes, about 24 MB/s
insert into select: 29.2 seconds inserted 10000000 Rows, about 342K ops/s
storage size: 17163974810 Bytes

jacktengg · 2023-12-04T16:25:01Z

run buildall

…tic precision promotion 1. [DNM](decimal) use new way for decimal arithmetic precision promotion 2. throw exception if it overflows for decimal arithmetics 3. throw exception if it overflows when casting among number types

jacktengg · 2023-12-04T16:29:20Z

run buildall

github-actions

clang-tidy made some suggestions

github-actions · 2023-12-04T16:32:03Z

 /// there's implicit type conversion here
- static ALWAYS_INLINE NativeResultType apply(NativeResultType a, NativeResultType b) {
+ template <bool need_adjust_scale>
+ static ALWAYS_INLINE NativeResultType apply(NativeResultType a, NativeResultType b,


warning: function 'apply' has cognitive complexity of 61 (threshold 50) [readability-function-cognitive-complexity]
staticALWAYS_INLINE NativeResultType apply(NativeResultType a, NativeResultType b, ^
Additional context
be/src/vec/functions/function_binary_arithmetic.h:519: +1, including nesting penalty of 0, nesting level increased to 1
ifconstexpr (IsDecimalV2<B> || IsDecimalV2<A>) { ^
be/src/vec/functions/function_binary_arithmetic.h:519: +1
ifconstexpr (IsDecimalV2<B> || IsDecimalV2<A>) { ^
be/src/vec/functions/function_binary_arithmetic.h:523: +1, nesting level increased to 1
} else { ^
be/src/vec/functions/function_binary_arithmetic.h:525: +2, including nesting penalty of 1, nesting level increased to 2
ifconstexpr (OpTraits::can_overflow && check_overflow) { ^
be/src/vec/functions/function_binary_arithmetic.h:527: +3, including nesting penalty of 2, nesting level increased to 3
if (UNLIKELY(Op::template apply<NativeResultType>(a, b, res))) { ^
be/src/vec/functions/function_binary_arithmetic.h:528: +4, including nesting penalty of 3, nesting level increased to 4
ifconstexpr (OpTraits::is_plus_minus) { ^
be/src/vec/functions/function_binary_arithmetic.h:533: +4, including nesting penalty of 3, nesting level increased to 4
ifconstexpr (std::is_same_v<NativeResultType, __int128>) { ^
be/src/vec/functions/function_binary_arithmetic.h:535: +5, including nesting penalty of 4, nesting level increased to 5
ifconstexpr (OpTraits::is_multiply && need_adjust_scale) { ^
be/src/vec/functions/function_binary_arithmetic.h:536: +6, including nesting penalty of 5, nesting level increased to 6
if (res256 > 0) { ^
be/src/vec/functions/function_binary_arithmetic.h:540: +1, nesting level increased to 6
} else { ^
be/src/vec/functions/function_binary_arithmetic.h:546: +5, including nesting penalty of 4, nesting level increased to 5
if (res256 > wide::Int256(max_result_number.value) || ^
be/src/vec/functions/function_binary_arithmetic.h:546: +1
if (res256 > wide::Int256(max_result_number.value) || ^
be/src/vec/functions/function_binary_arithmetic.h:550: +1, nesting level increased to 5
} else { ^
be/src/vec/functions/function_binary_arithmetic.h:553: +1, nesting level increased to 4
} else { ^
be/src/vec/functions/function_binary_arithmetic.h:557: +1, nesting level increased to 3
} else { ^
be/src/vec/functions/function_binary_arithmetic.h:559: +4, including nesting penalty of 3, nesting level increased to 4
ifconstexpr (OpTraits::is_multiply && need_adjust_scale) { ^
be/src/vec/functions/function_binary_arithmetic.h:560: +5, including nesting penalty of 4, nesting level increased to 5
if (res >= 0) { ^
be/src/vec/functions/function_binary_arithmetic.h:563: +1, nesting level increased to 5
} else { ^
be/src/vec/functions/function_binary_arithmetic.h:568: +4, including nesting penalty of 3, nesting level increased to 4
if (res > max_result_number.value || ^
be/src/vec/functions/function_binary_arithmetic.h:568: +1
if (res > max_result_number.value || ^
be/src/vec/functions/function_binary_arithmetic.h:575: +1, nesting level increased to 2
} else { ^
be/src/vec/functions/function_binary_arithmetic.h:577: +3, including nesting penalty of 2, nesting level increased to 3
ifconstexpr (OpTraits::is_multiply && need_adjust_scale) { ^
be/src/vec/functions/function_binary_arithmetic.h:578: +4, including nesting penalty of 3, nesting level increased to 4
if (res >= 0) { ^
be/src/vec/functions/function_binary_arithmetic.h:580: +1, nesting level increased to 4
} else { ^

github-actions

clang-tidy made some suggestions

github-actions · 2023-12-04T16:36:46Z

 /// there's implicit type conversion here
- static ALWAYS_INLINE NativeResultType apply(NativeResultType a, NativeResultType b) {
+ template <bool need_adjust_scale>
+ static ALWAYS_INLINE NativeResultType apply(NativeResultType a, NativeResultType b,


warning: function 'apply' has cognitive complexity of 61 (threshold 50) [readability-function-cognitive-complexity]
staticALWAYS_INLINE NativeResultType apply(NativeResultType a, NativeResultType b, ^
Additional context
be/src/vec/functions/function_binary_arithmetic.h:519: +1, including nesting penalty of 0, nesting level increased to 1
ifconstexpr (IsDecimalV2<B> || IsDecimalV2<A>) { ^
be/src/vec/functions/function_binary_arithmetic.h:519: +1
ifconstexpr (IsDecimalV2<B> || IsDecimalV2<A>) { ^
be/src/vec/functions/function_binary_arithmetic.h:523: +1, nesting level increased to 1
} else { ^
be/src/vec/functions/function_binary_arithmetic.h:525: +2, including nesting penalty of 1, nesting level increased to 2
ifconstexpr (OpTraits::can_overflow && check_overflow) { ^
be/src/vec/functions/function_binary_arithmetic.h:527: +3, including nesting penalty of 2, nesting level increased to 3
if (UNLIKELY(Op::template apply<NativeResultType>(a, b, res))) { ^
be/src/vec/functions/function_binary_arithmetic.h:528: +4, including nesting penalty of 3, nesting level increased to 4
ifconstexpr (OpTraits::is_plus_minus) { ^
be/src/vec/functions/function_binary_arithmetic.h:533: +4, including nesting penalty of 3, nesting level increased to 4
ifconstexpr (std::is_same_v<NativeResultType, __int128>) { ^
be/src/vec/functions/function_binary_arithmetic.h:535: +5, including nesting penalty of 4, nesting level increased to 5
ifconstexpr (OpTraits::is_multiply && need_adjust_scale) { ^
be/src/vec/functions/function_binary_arithmetic.h:536: +6, including nesting penalty of 5, nesting level increased to 6
if (res256 > 0) { ^
be/src/vec/functions/function_binary_arithmetic.h:540: +1, nesting level increased to 6
} else { ^
be/src/vec/functions/function_binary_arithmetic.h:546: +5, including nesting penalty of 4, nesting level increased to 5
if (res256 > wide::Int256(max_result_number.value) || ^
be/src/vec/functions/function_binary_arithmetic.h:546: +1
if (res256 > wide::Int256(max_result_number.value) || ^
be/src/vec/functions/function_binary_arithmetic.h:550: +1, nesting level increased to 5
} else { ^
be/src/vec/functions/function_binary_arithmetic.h:553: +1, nesting level increased to 4
} else { ^
be/src/vec/functions/function_binary_arithmetic.h:557: +1, nesting level increased to 3
} else { ^
be/src/vec/functions/function_binary_arithmetic.h:559: +4, including nesting penalty of 3, nesting level increased to 4
ifconstexpr (OpTraits::is_multiply && need_adjust_scale) { ^
be/src/vec/functions/function_binary_arithmetic.h:560: +5, including nesting penalty of 4, nesting level increased to 5
if (res >= 0) { ^
be/src/vec/functions/function_binary_arithmetic.h:563: +1, nesting level increased to 5
} else { ^
be/src/vec/functions/function_binary_arithmetic.h:568: +4, including nesting penalty of 3, nesting level increased to 4
if (res > max_result_number.value || res < -max_result_number.value) { ^
be/src/vec/functions/function_binary_arithmetic.h:568: +1
if (res > max_result_number.value || res < -max_result_number.value) { ^
be/src/vec/functions/function_binary_arithmetic.h:574: +1, nesting level increased to 2
} else { ^
be/src/vec/functions/function_binary_arithmetic.h:576: +3, including nesting penalty of 2, nesting level increased to 3
ifconstexpr (OpTraits::is_multiply && need_adjust_scale) { ^
be/src/vec/functions/function_binary_arithmetic.h:577: +4, including nesting penalty of 3, nesting level increased to 4
if (res >= 0) { ^
be/src/vec/functions/function_binary_arithmetic.h:579: +1, nesting level increased to 4
} else { ^

jacktengg · 2023-12-04T17:45:47Z

run feut

github-actions · 2023-12-05T01:50:51Z

PR approved by at least one committer and no changes requested.

github-actions · 2023-12-05T01:50:53Z

PR approved by anyone and no changes requested.

yiguolei

LGTM

…romotion (apache#27787) * [DNM](decimal) use new way for decimal arithmetic precision promotion * [improvement](decimal) [DNM](decimal) use new way for decimal arithmetic precision promotion 1. [DNM](decimal) use new way for decimal arithmetic precision promotion 2. throw exception if it overflows for decimal arithmetics 3. throw exception if it overflows when casting among number types * fix compile error of gcc * improvement --------- Co-authored-by: morrySnow <morrysnow@126.com>

wm1581066 requested review from Gabriel39 and morrySnow November 30, 2023 01:45

github-actionsBot reviewed Nov 30, 2023
View reviewed changes

Comment threadbe/src/vec/common/int_exp.h

yiguolei reviewed Nov 30, 2023
View reviewed changes

Comment threadbe/src/vec/core/types.h

yiguolei reviewed Nov 30, 2023
View reviewed changes

xiaokang added the dev/2.0.4 label Nov 30, 2023

jacktengg force-pushed the decimal_overflow_merge-rebase branch from ce957ef to 531a3b1Compare November 30, 2023 16:05

jacktengg force-pushed the decimal_overflow_merge-rebase branch 2 times, most recently from ccccbcb to c691eafCompare December 1, 2023 01:35

jacktengg marked this pull request as ready for review December 1, 2023 01:35

jacktengg force-pushed the decimal_overflow_merge-rebase branch 2 times, most recently from 603533a to a069f25Compare December 3, 2023 15:17

jacktengg force-pushed the decimal_overflow_merge-rebase branch 2 times, most recently from 3eb6ea5 to 59a633eCompare December 4, 2023 01:26

jacktengg force-pushed the decimal_overflow_merge-rebase branch from 3608c81 to 0676754Compare December 4, 2023 07:11

Gabriel39 reviewed Dec 4, 2023
View reviewed changes

jacktengg force-pushed the decimal_overflow_merge-rebase branch from 0676754 to e01c532Compare December 4, 2023 07:12

jacktengg force-pushed the decimal_overflow_merge-rebase branch 2 times, most recently from 61c9d6a to 8c126c0Compare December 4, 2023 16:24

jacktengg added 12 commits December 5, 2023 00:29

fix
bb1ecf8

fix coredump for decimalv2
643f6c7

fix add result type
19d9ec3

improve and fix test cases
dc408ff

fix
08e6417

fix
bf7296f

fix be ut failure
b3b7fcd

fix test cases
846bcbb

fix more cases
c093b35

fix compile error of gcc
509f7cf

improvement
4f14b9a

jacktengg force-pushed the decimal_overflow_merge-rebase branch from 55be2d3 to 4f14b9aCompare December 4, 2023 16:29

github-actionsBot reviewed Dec 4, 2023
View reviewed changes

morrySnow approved these changes Dec 5, 2023
View reviewed changes

github-actionsBot added the approved Indicates a PR has been approved by one committer. label Dec 5, 2023

github-actionsBot added the reviewed label Dec 5, 2023

yiguolei approved these changes Dec 5, 2023
View reviewed changes

yiguolei merged commit 17016b9 into apache:masterDec 5, 2023

yiguolei added the kind/behavior-changed label Dec 5, 2023

jacktengg mentioned this pull request Dec 5, 2023
[improvement](decimal) use new way for decimal arithmetic precision promotion(#27787) #28034
Merged

xiaokang added dev/2.0.4-merged and removed dev/2.0.4 labels Jan 14, 2024

jacktengg mentioned this pull request Dec 5, 2024
[Bug] base of 25451 issue , use union all will influence result #25452
Closed
3 tasks

Uh oh!

Conversation

jacktengg commented Nov 30, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Further comments

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yiguoleiNov 30, 2023

Choose a reason for hiding this comment

Uh oh!

yiguoleiNov 30, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yiguoleiNov 30, 2023

Choose a reason for hiding this comment

Uh oh!

jacktengg commented Nov 30, 2023

Uh oh!

jacktengg commented Dec 1, 2023

Uh oh!

jacktengg commented Dec 3, 2023

Uh oh!

jacktengg commented Dec 4, 2023

Uh oh!

jacktengg commented Dec 4, 2023

Uh oh!

jacktengg commented Dec 4, 2023

Uh oh!

Gabriel39Dec 4, 2023

Choose a reason for hiding this comment

Uh oh!

jacktengg commented Dec 4, 2023

Uh oh!

doris-robot commented Dec 4, 2023

Uh oh!

jacktengg commented Dec 4, 2023

Uh oh!

jacktengg commented Dec 4, 2023

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actionsBotDec 4, 2023

Choose a reason for hiding this comment

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actionsBotDec 4, 2023

Choose a reason for hiding this comment

Uh oh!

jacktengg commented Dec 4, 2023

Uh oh!

github-actionsBot commented Dec 5, 2023

Uh oh!

github-actionsBot commented Dec 5, 2023

Uh oh!

yiguolei left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jacktengg commented Nov 30, 2023•
edited
Loading