You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Xuebin Su (Code Review)" <ge...@cloudera.org> on 2024/09/05 08:39:05 UTC
[Impala-ASF-CR] IMPALA-13287: Check overflow for bigint arithmetic
Xuebin Su has uploaded this change for review. ( http://gerrit.cloudera.org:8080/21755
Change subject: IMPALA-13287: Check overflow for bigint arithmetic
......................................................................
IMPALA-13287: Check overflow for bigint arithmetic
Previously, Impala would return a wrong query result for bigint
arithmetic if the result exceeded the limits of bigint due to overflow.
This patch fixes the issue by adding a check in each bigint arithmetic
operation, and reporting an error if the result overflowed, rather than
returning the wrong result. The check is implemented using the compiler
built-in functions for efficiency.
Testing
- Added test cases for bigint addition, substraction, and
multiplication in the query test.
Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
---
M be/src/exprs/operators-ir.cc
M testdata/workloads/functional-query/queries/QueryTest/overflow.test
2 files changed, 48 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/21755/1
--
To view, visit http://gerrit.cloudera.org:8080/21755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
Gerrit-Change-Number: 21755
Gerrit-PatchSet: 1
Gerrit-Owner: Xuebin Su <xs...@cloudera.com>
[Impala-ASF-CR] IMPALA-13287: Check overflow for bigint arithmetic
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21755 )
Change subject: IMPALA-13287: Check overflow for bigint arithmetic
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
Any notable performance impact to this?
http://gerrit.cloudera.org:8080/#/c/21755/1/be/src/exprs/operators-ir.cc
File be/src/exprs/operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/21755/1/be/src/exprs/operators-ir.cc@165
PS1, Line 165: #define BINARY_OP_CHECK_OVERFLOW(TYPE, TYPE_NAME) \
> Thanks! Fixed.
Ack
--
To view, visit http://gerrit.cloudera.org:8080/21755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
Gerrit-Change-Number: 21755
Gerrit-PatchSet: 2
Gerrit-Owner: Xuebin Su <xs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Xuebin Su <xs...@cloudera.com>
Gerrit-Comment-Date: Fri, 20 Sep 2024 18:04:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13287: Check overflow for bigint arithmetic
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21755 )
Change subject: IMPALA-13287: Check overflow for bigint arithmetic
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21755/3/be/src/benchmarks/expr-benchmark.cc
File be/src/benchmarks/expr-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/21755/3/be/src/benchmarks/expr-benchmark.cc@241
PS3, Line 241: // Arithmetic: Function iters/ms 10%ile 50%ile 90%ile 10%ile 50%ile 90%ile
: // (relative) (relative) (relative)
: // ---------------------------------------------------------------------------------------------------------
: // int-add 319 323 324 1X 1X 1X
: // double-add 40 40.3 40.5 0.125X 0.125X 0.125X
: //
: // ArithmeticCodegen: Function iters/ms 10%ile 50%ile 90%ile 10%ile 50%ile 90%ile
: // (relative) (relative) (relative)
: // ---------------------------------------------------------------------------------------------------------
: // int-add 781 787 791 1X 1X 1X
: // double-add 610 617 619 0.781X 0.784X 0.783X
Thank you for adding the microbenchmark. Please update this BenchmarkArithmetic table with result from your latest run.
I usually do following before running this microbenchmark to get consistent result:
sudo cpupower frequency-set --governor performance
echo "1" | sudo tee /sys/devices/system/cpu/intel_pstate/no_turbo
Also, make sure the impala and microbenchmarking code is from RELEASE build.
./buildall.sh -skiptests -release
--
To view, visit http://gerrit.cloudera.org:8080/21755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
Gerrit-Change-Number: 21755
Gerrit-PatchSet: 3
Gerrit-Owner: Xuebin Su <xs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Xuebin Su <xs...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Oct 2024 17:34:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13287: Check overflow for bigint arithmetic
Posted by "Xuebin Su (Code Review)" <ge...@cloudera.org>.
Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/21755 )
Change subject: IMPALA-13287: Check overflow for bigint arithmetic
......................................................................
Patch Set 2:
(1 comment)
> Patch Set 1:
>
> (1 comment)
Thanks for reviewing!
http://gerrit.cloudera.org:8080/#/c/21755/1/be/src/exprs/operators-ir.cc
File be/src/exprs/operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/21755/1/be/src/exprs/operators-ir.cc@165
PS1, Line 165: #define BINARY_OP_CHECK_OVERFLOW(TYPE, TYPE_NAME) \
> typo: CHCEK should be CHECK
Thanks! Fixed.
--
To view, visit http://gerrit.cloudera.org:8080/21755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
Gerrit-Change-Number: 21755
Gerrit-PatchSet: 2
Gerrit-Owner: Xuebin Su <xs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Xuebin Su <xs...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Sep 2024 07:10:39 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13287: Check overflow for bigint arithmetic
Posted by "Xuebin Su (Code Review)" <ge...@cloudera.org>.
Xuebin Su has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/21755 )
Change subject: IMPALA-13287: Check overflow for bigint arithmetic
......................................................................
IMPALA-13287: Check overflow for bigint arithmetic
Previously, Impala would return a wrong query result for bigint
arithmetic if the result exceeded the limits of bigint due to overflow.
This patch fixes the issue by adding a check in each bigint arithmetic
operation, and reporting an error if the result overflowed, rather than
returning the wrong result. The check is implemented using the compiler
built-in functions for efficiency.
Testing
- Added test cases for bigint addition, substraction, and
multiplication in the query test.
Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
---
M be/src/benchmarks/expr-benchmark.cc
M be/src/exprs/operators-ir.cc
M testdata/workloads/functional-query/queries/QueryTest/overflow.test
3 files changed, 72 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/21755/3
--
To view, visit http://gerrit.cloudera.org:8080/21755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
Gerrit-Change-Number: 21755
Gerrit-PatchSet: 3
Gerrit-Owner: Xuebin Su <xs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Xuebin Su <xs...@cloudera.com>
[Impala-ASF-CR] IMPALA-13287: Check overflow for bigint arithmetic
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21755 )
Change subject: IMPALA-13287: Check overflow for bigint arithmetic
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17248/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/21755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
Gerrit-Change-Number: 21755
Gerrit-PatchSet: 3
Gerrit-Owner: Xuebin Su <xs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Xuebin Su <xs...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Oct 2024 08:35:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13287: Check overflow for bigint arithmetic
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21755 )
Change subject: IMPALA-13287: Check overflow for bigint arithmetic
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/16884/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/21755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
Gerrit-Change-Number: 21755
Gerrit-PatchSet: 1
Gerrit-Owner: Xuebin Su <xs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 05 Sep 2024 09:10:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13287: Check overflow for bigint arithmetic
Posted by "Xuebin Su (Code Review)" <ge...@cloudera.org>.
Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/21755 )
Change subject: IMPALA-13287: Check overflow for bigint arithmetic
......................................................................
Patch Set 3:
(1 comment)
Thanks for your feedbacks!
http://gerrit.cloudera.org:8080/#/c/21755/2/testdata/workloads/functional-query/queries/QueryTest/overflow.test
File testdata/workloads/functional-query/queries/QueryTest/overflow.test:
http://gerrit.cloudera.org:8080/#/c/21755/2/testdata/workloads/functional-query/queries/QueryTest/overflow.test@14
PS2, Line 14: ---- QUERY: Bigint addition overflow
: select 9223372036854775807 + 1;
: ---- CATCH
: Bigint expression overflowed
: ====
: ---- QUERY: Bigint subtraction overflow
: select -9223372036854775808 - 1;
: ---- CATCH
: Bigint expression overflowed
: ====
: ---- QUERY: Bigint multiplication overflow
: # 4294967296 is 2^32
: select 4294967296 * 4294967296;
: ---- CATCH
: Bigint expression overflowed
: ====
> Can you add these and non-overflow expression into expr-benchmark.cc, under
Hi! I added new microbenchmarks in expr-benchmark. On my machine, they show that
- in the non-overflow case, the preformance almost does not change, while
- in the overflow case, the performance drops by about 15%. I think the extra time is mainly spent on error reporting. In the microbenchmark, error reporting happens many times (in `FunctionContext::SetError()`), while in the real world, it will only happen once per query.
What do you think? Is my understanding correct? Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/21755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
Gerrit-Change-Number: 21755
Gerrit-PatchSet: 3
Gerrit-Owner: Xuebin Su <xs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Xuebin Su <xs...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 Oct 2024 08:18:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13287: Check overflow for bigint arithmetic
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/21755 )
Change subject: IMPALA-13287: Check overflow for bigint arithmetic
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21755/1/be/src/exprs/operators-ir.cc
File be/src/exprs/operators-ir.cc:
http://gerrit.cloudera.org:8080/#/c/21755/1/be/src/exprs/operators-ir.cc@165
PS1, Line 165: #define BINARY_OP_CHCEK_OVERFLOW(TYPE, TYPE_NAME) \
typo: CHCEK should be CHECK
--
To view, visit http://gerrit.cloudera.org:8080/21755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
Gerrit-Change-Number: 21755
Gerrit-PatchSet: 1
Gerrit-Owner: Xuebin Su <xs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Sep 2024 18:14:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13287: Check overflow for bigint arithmetic
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/21755 )
Change subject: IMPALA-13287: Check overflow for bigint arithmetic
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/16977/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/21755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
Gerrit-Change-Number: 21755
Gerrit-PatchSet: 2
Gerrit-Owner: Xuebin Su <xs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Xuebin Su <xs...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Sep 2024 07:42:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13287: Check overflow for bigint arithmetic
Posted by "Xuebin Su (Code Review)" <ge...@cloudera.org>.
Xuebin Su has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/21755 )
Change subject: IMPALA-13287: Check overflow for bigint arithmetic
......................................................................
IMPALA-13287: Check overflow for bigint arithmetic
Previously, Impala would return a wrong query result for bigint
arithmetic if the result exceeded the limits of bigint due to overflow.
This patch fixes the issue by adding a check in each bigint arithmetic
operation, and reporting an error if the result overflowed, rather than
returning the wrong result. The check is implemented using the compiler
built-in functions for efficiency.
Testing
- Added test cases for bigint addition, substraction, and
multiplication in the query test.
Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
---
M be/src/exprs/operators-ir.cc
M testdata/workloads/functional-query/queries/QueryTest/overflow.test
2 files changed, 48 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/55/21755/2
--
To view, visit http://gerrit.cloudera.org:8080/21755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
Gerrit-Change-Number: 21755
Gerrit-PatchSet: 2
Gerrit-Owner: Xuebin Su <xs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-13287: Check overflow for bigint arithmetic
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21755 )
Change subject: IMPALA-13287: Check overflow for bigint arithmetic
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/21755/2/testdata/workloads/functional-query/queries/QueryTest/overflow.test
File testdata/workloads/functional-query/queries/QueryTest/overflow.test:
http://gerrit.cloudera.org:8080/#/c/21755/2/testdata/workloads/functional-query/queries/QueryTest/overflow.test@14
PS2, Line 14: ---- QUERY: Bigint addition overflow
: select 9223372036854775807 + 1;
: ---- CATCH
: Bigint expression overflowed
: ====
: ---- QUERY: Bigint subtraction overflow
: select -9223372036854775808 - 1;
: ---- CATCH
: Bigint expression overflowed
: ====
: ---- QUERY: Bigint multiplication overflow
: # 4294967296 is 2^32
: select 4294967296 * 4294967296;
: ---- CATCH
: Bigint expression overflowed
: ====
Can you add these and non-overflow expression into expr-benchmark.cc, under BenchmarkArithmetic?
Please check how the number compares before and after changes.
--
To view, visit http://gerrit.cloudera.org:8080/21755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
Gerrit-Change-Number: 21755
Gerrit-PatchSet: 2
Gerrit-Owner: Xuebin Su <xs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Xuebin Su <xs...@cloudera.com>
Gerrit-Comment-Date: Sat, 19 Oct 2024 05:19:11 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13287: Check overflow for bigint arithmetic
Posted by "Xuebin Su (Code Review)" <ge...@cloudera.org>.
Xuebin Su has posted comments on this change. ( http://gerrit.cloudera.org:8080/21755 )
Change subject: IMPALA-13287: Check overflow for bigint arithmetic
......................................................................
Patch Set 2:
> Patch Set 2:
>
> (1 comment)
Thank you very much for the feedbacks!
I am working on it.
--
To view, visit http://gerrit.cloudera.org:8080/21755
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf22bef737caeab241ac3c708f43c5a72bb22350
Gerrit-Change-Number: 21755
Gerrit-PatchSet: 2
Gerrit-Owner: Xuebin Su <xs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Xuebin Su <xs...@cloudera.com>
Gerrit-Comment-Date: Mon, 21 Oct 2024 07:52:05 +0000
Gerrit-HasComments: No