[function](round) compute accurate round value by decimal by Gabriel39 · Pull Request #14946 · apache/doris

Gabriel39 · 2022-12-08T13:04:54Z

Proposed changes

before:

after:

Problem summary

Describe your changes.

Checklist(Required)

Does it affect the original behavior:
- Yes
- No
- I don't know
Has unit tests been added:
- Yes
- No
- No Need
Has document been added or modified:
- Yes
- No
- No Need
Does it need to update dependencies:
- Yes
- No
Are there any changes that cannot be rolled back:
- Yes (If Yes, please explain WHY)
- No

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

github-actions · 2022-12-08T13:08:27Z

be/src/vec/functions/round.h

+            return x / scale * scale;
+        }
+        case RoundingMode::Floor: {
+            if (x < 0) x -= scale - 1;


warning: statement should be inside braces [readability-braces-around-statements]

Suggested change

if (x < 0) x -= scale - 1;

if (x < 0) { x -= scale - 1;

}

github-actions · 2022-12-08T13:08:28Z

be/src/vec/functions/round.h

+            return x / scale * scale;
+        }
+        case RoundingMode::Ceil: {
+            if (x >= 0) x += scale - 1;


warning: statement should be inside braces [readability-braces-around-statements]

Suggested change

if (x >= 0) x += scale - 1;

if (x >= 0) { x += scale - 1;

}

github-actions · 2022-12-08T13:08:28Z

be/src/vec/functions/round.h

+            return x / scale * scale;
+        }
+        case RoundingMode::Round: {
+            if (x < 0) x -= scale;


warning: statement should be inside braces [readability-braces-around-statements]

Suggested change

if (x < 0) x -= scale;

if (x < 0) { x -= scale;

}

github-actions · 2022-12-08T13:08:28Z

be/src/vec/functions/round.h

+                break;
+            case TieBreakingMode::Bankers: {
+                T quotient = (x + scale / 2) / scale;
+                if (quotient * scale == x + scale / 2)


warning: statement should be inside braces [readability-braces-around-statements]

Suggested change

if (quotient * scale == x + scale / 2)

if (quotient * scale == x + scale / 2) {

be/src/vec/functions/round.h:92:

- else + } else

github-actions · 2022-12-08T13:08:28Z

be/src/vec/functions/round.h

+                if (quotient * scale == x + scale / 2)
+                    // round half to even
+                    x = ((quotient + (x < 0)) & ~1) * scale;
+                else


warning: statement should be inside braces [readability-braces-around-statements]

Suggested change

else

else {

be/src/vec/functions/round.h:94:

- x = quotient * scale; + x = quotient * scale; + }

github-actions · 2022-12-08T13:08:31Z

be/src/vec/functions/round.h

+    }
+
+    bool use_default_implementation_for_constants() const override { return true; }
+    ColumnNumbers get_arguments_that_are_always_constant() const override { return {1}; }


warning: unknown type name 'ColumnNumbers' [clang-diagnostic-error]

ColumnNumbers get_arguments_that_are_always_constant() const override { return {1}; } ^

github-actions · 2022-12-08T13:08:31Z

be/src/vec/functions/round.h

+    }
+
+    bool use_default_implementation_for_constants() const override { return true; }
+    ColumnNumbers get_arguments_that_are_always_constant() const override { return {1}; }


warning: braces around scalar initializer [clang-diagnostic-braced-scalar-init]

Suggested change

ColumnNumbers get_arguments_that_are_always_constant() const override { return {1}; }

ColumnNumbers get_arguments_that_are_always_constant() const override { return 1; }

github-actions · 2022-12-08T13:08:32Z

be/src/vec/functions/round.h

+    bool use_default_implementation_for_constants() const override { return true; }
+    ColumnNumbers get_arguments_that_are_always_constant() const override { return {1}; }
+
+    Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments,


warning: unknown type name 'Block' [clang-diagnostic-error]

Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments, ^

github-actions · 2022-12-08T13:08:32Z

be/src/vec/functions/round.h

+    bool use_default_implementation_for_constants() const override { return true; }
+    ColumnNumbers get_arguments_that_are_always_constant() const override { return {1}; }
+
+    Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments,


warning: unknown type name 'ColumnNumbers' [clang-diagnostic-error]

Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments, ^

github-actions · 2022-12-08T13:08:32Z

be/src/vec/functions/round.h

+
+    Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments,
+                        size_t result, size_t /*input_rows_count*/) override {
+        const ColumnWithTypeAndName& column = block.get_by_position(arguments[0]);


warning: unknown type name 'ColumnWithTypeAndName' [clang-diagnostic-error]

const ColumnWithTypeAndName& column = block.get_by_position(arguments[0]); ^

hello-stephen · 2022-12-08T15:11:18Z

TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 35.38 seconds
load time: 480 seconds
storage size: 17123356232 Bytes
https://doris-community-test-1308700295.cos.ap-hongkong.myqcloud.com/tmp/20221212043532_clickbench_pr_61804.html

github-actions

clang-tidy made some suggestions

github-actions · 2022-12-09T01:49:28Z

be/src/vec/functions/round.h

+            return x / scale * scale;
+        }
+        case RoundingMode::Floor: {
+            if (x < 0) {


warning: statement should be inside braces [readability-braces-around-statements]

Suggested change

if (x < 0) {

if (x < 0) { x -= scale - 1;

}

github-actions · 2022-12-09T01:49:28Z

be/src/vec/functions/round.h

+                x -= scale - 1;
+            }
+            return x / scale * scale;
+        }


warning: statement should be inside braces [readability-braces-around-statements]

Suggested change

}

if (x >= 0) { x += scale - 1;

}

github-actions · 2022-12-09T01:49:29Z

be/src/vec/functions/round.h

+        case RoundingMode::Ceil: {
+            if (x >= 0) {
+                x += scale - 1;
+            }


warning: statement should be inside braces [readability-braces-around-statements]

Suggested change

}

if (x < 0) { x -= scale;

}

github-actions · 2022-12-09T01:49:29Z

be/src/vec/functions/round.h

+            if (x < 0) {
+                x -= scale;
+            }
+            switch (tie_breaking_mode) {


warning: statement should be inside braces [readability-braces-around-statements]

Suggested change

switch (tie_breaking_mode) {

if (quotient * scale == x + scale / 2) {

be/src/vec/functions/round.h:92:

- else + } else

github-actions · 2022-12-09T01:49:29Z

be/src/vec/functions/round.h

+            switch (tie_breaking_mode) {
+            case TieBreakingMode::Auto:
+                x = (x + scale / 2) / scale * scale;
+                break;


warning: statement should be inside braces [readability-braces-around-statements]

Suggested change

break;

else {

be/src/vec/functions/round.h:94:

- x = quotient * scale; + x = quotient * scale; + }

github-actions · 2022-12-09T01:49:32Z

be/src/vec/functions/round.h

+
+        Int64 scale64 = scale_field.get<Int64>();
+        if (scale64 > std::numeric_limits<Int16>::max() ||
+            scale64 < std::numeric_limits<Int16>::min()) {


warning: unknown type name 'ColumnNumbers' [clang-diagnostic-error]

ColumnNumbers get_arguments_that_are_always_constant() const override { return {1}; } ^

github-actions · 2022-12-09T01:49:32Z

be/src/vec/functions/round.h

+
+        Int64 scale64 = scale_field.get<Int64>();
+        if (scale64 > std::numeric_limits<Int16>::max() ||
+            scale64 < std::numeric_limits<Int16>::min()) {


warning: braces around scalar initializer [clang-diagnostic-braced-scalar-init]

Suggested change

scale64 < std::numeric_limits<Int16>::min()) {

ColumnNumbers get_arguments_that_are_always_constant() const override { return 1; }

github-actions · 2022-12-09T01:49:32Z

be/src/vec/functions/round.h

+        if (scale64 > std::numeric_limits<Int16>::max() ||
+            scale64 < std::numeric_limits<Int16>::min()) {
+            return Status::InvalidArgument("Scale argument for function {} is too large: {}", name,
+                                           scale64);


warning: unknown type name 'Block' [clang-diagnostic-error]

Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments, ^

github-actions · 2022-12-09T01:49:32Z

be/src/vec/functions/round.h

+        if (scale64 > std::numeric_limits<Int16>::max() ||
+            scale64 < std::numeric_limits<Int16>::min()) {
+            return Status::InvalidArgument("Scale argument for function {} is too large: {}", name,
+                                           scale64);


warning: unknown type name 'ColumnNumbers' [clang-diagnostic-error]

Status execute_impl(FunctionContext* context, Block& block, const ColumnNumbers& arguments, ^

github-actions · 2022-12-09T01:49:32Z

be/src/vec/functions/round.h

+            return Status::InvalidArgument("Scale argument for function {} is too large: {}", name,
+                                           scale64);
+        }
+


warning: unknown type name 'ColumnWithTypeAndName' [clang-diagnostic-error]

const ColumnWithTypeAndName& column = block.get_by_position(arguments[0]); ^

HappenLee · 2022-12-12T02:58:34Z

be/src/vec/functions/round.h

+    static size_t prepare(size_t scale) { return scale; }
+
+    /// Integer overflow is Ok.
+    static ALWAYS_INLINE T computeImpl(T x, T scale) {


compute_impl

HappenLee

LGTM

github-actions · 2022-12-12T16:07:22Z

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

github-actions · 2022-12-12T16:07:26Z

PR approved by anyone and no changes requested.

yangzhg · 2022-12-20T08:58:38Z

round(16.025,2) should be 16.03 not 16.03000

github-actions bot added area/planner Issues or PRs related to the query planner area/sql/function Issues or PRs related to the SQL functions area/vectorization kind/test labels Dec 8, 2022

github-actions bot reviewed Dec 8, 2022

View reviewed changes

[function](round) compute accurate round value by decimal

0fdb5df

yiguolei added dev/1.2.1 and removed dev/1.2.1 labels Dec 9, 2022

update

1e65710

Gabriel39 force-pushed the round_fn branch from 868b5b5 to 1e65710 Compare December 9, 2022 01:32

Gabriel39 added 2 commits December 9, 2022 09:45

update

3b44b08

update

48ce495

github-actions bot reviewed Dec 9, 2022

View reviewed changes

HappenLee reviewed Dec 12, 2022

View reviewed changes

update

9d3aa2b

HappenLee approved these changes Dec 12, 2022

View reviewed changes

github-actions bot added the approved Indicates a PR has been approved by one committer. label Dec 12, 2022

github-actions bot added the reviewed label Dec 12, 2022

Gabriel39 merged commit 1200b22 into apache:master Dec 13, 2022

Gabriel39 added a commit to Gabriel39/incubator-doris that referenced this pull request Dec 15, 2022

[function](round) compute accurate round value by decimal (apache#14946)

93cdfb7

morningman mentioned this pull request Dec 15, 2022

[cherrypick](datev2-decimalv3) refine function expr of datev2 & compute accurate round value by decimal #15103

Merged

13 tasks

morningman added dev/1.2.1-merged and removed dev/1.2.1 labels Dec 15, 2022

Gabriel39 added a commit to Gabriel39/incubator-doris that referenced this pull request Dec 18, 2022

[function](round) compute accurate round value by decimal (apache#14946)

b092de2

yangzhg mentioned this pull request Dec 20, 2022

[Bugfix](round) fix round function may coredump #15203

Merged

13 tasks

Gabriel39 mentioned this pull request Dec 22, 2022

[Bug] Use correct decimal scale for function round #15307

Closed

3 tasks

	if (quotient * scale == x + scale / 2)
	if (quotient * scale == x + scale / 2) {

	ColumnNumbers get_arguments_that_are_always_constant() const override { return {1}; }
	ColumnNumbers get_arguments_that_are_always_constant() const override { return 1; }

	switch (tie_breaking_mode) {
	if (quotient * scale == x + scale / 2) {

	scale64 < std::numeric_limits<Int16>::min()) {
	ColumnNumbers get_arguments_that_are_always_constant() const override { return 1; }

Conversation

Gabriel39 commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Problem summary

Checklist(Required)

Further comments

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

hello-stephen commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

HappenLee Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

Gabriel39 commented Dec 8, 2022 •

edited

Loading

hello-stephen commented Dec 8, 2022 •

edited

Loading