You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Jim Apple (Code Review)" <ge...@cloudera.org> on 2018/10/06 03:44:14 UTC

[Impala-ASF-CR] IMPALA-5031: fix signed overflows in decimal 

Jim Apple has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11604


Change subject: IMPALA-5031: fix signed overflows in decimal
......................................................................

IMPALA-5031: fix signed overflows in decimal

The standard says that overflow for signed arithmetic operations is
undefined behavior; see [expr]:

    If during the evaluation of an expression, the result is not
    mathematically defined or not in the range of representable values
    for its type, the behavior is undefined.

and [basic.fundamental]:

    Unsigned integers shall obey the laws of arithmetic modulo 2^n
    where n is the number of bits in the value representation of that
    particular size of integer. This implies that unsigned arithmetic
    does not overflow because a result that cannot be represented by
    the resulting unsigned integer type is reduced modulo the number
    that is one greater than the largest value that can be represented
    by the resulting unsigned integer type.

All of the overflows fixed in this patch were tested with expr-test's
DecimalArithmeticTest.

Change-Id: Ibf882428931e4f4264be2fc8cd9d6b1fc89b8ace
---
M be/src/exprs/operators-ir.cc
M be/src/runtime/decimal-value.inline.h
M be/src/runtime/multi-precision.h
A be/src/util/arithmetic-util.h
M be/src/util/bit-util.h
M be/src/util/decimal-util.h
6 files changed, 145 insertions(+), 87 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/04/11604/1
-- 
To view, visit http://gerrit.cloudera.org:8080/11604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibf882428931e4f4264be2fc8cd9d6b1fc89b8ace
Gerrit-Change-Number: 11604
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>

[Impala-ASF-CR] IMPALA-5031: fix signed overflows in decimal

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11604 )

Change subject: IMPALA-5031: fix signed overflows in decimal
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/975/ : 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/11604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf882428931e4f4264be2fc8cd9d6b1fc89b8ace
Gerrit-Change-Number: 11604
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Comment-Date: Sat, 06 Oct 2018 04:32:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: fix signed overflows in decimal

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11604 )

Change subject: IMPALA-5031: fix signed overflows in decimal
......................................................................

IMPALA-5031: fix signed overflows in decimal

The standard says that overflow for signed arithmetic operations is
undefined behavior; see [expr]:

    If during the evaluation of an expression, the result is not
    mathematically defined or not in the range of representable values
    for its type, the behavior is undefined.

and [basic.fundamental]:

    Unsigned integers shall obey the laws of arithmetic modulo 2^n
    where n is the number of bits in the value representation of that
    particular size of integer. This implies that unsigned arithmetic
    does not overflow because a result that cannot be represented by
    the resulting unsigned integer type is reduced modulo the number
    that is one greater than the largest value that can be represented
    by the resulting unsigned integer type.

All of the overflows fixed in this patch were tested with expr-test's
DecimalArithmeticTest.

Change-Id: Ibf882428931e4f4264be2fc8cd9d6b1fc89b8ace
Reviewed-on: http://gerrit.cloudera.org:8080/11604
Reviewed-by: Jim Apple <jb...@apache.org>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exprs/operators-ir.cc
M be/src/runtime/decimal-value.inline.h
M be/src/runtime/multi-precision.h
A be/src/util/arithmetic-util.h
M be/src/util/bit-util.h
M be/src/util/decimal-util.h
6 files changed, 145 insertions(+), 87 deletions(-)

Approvals:
  Jim Apple: Looks good to me, approved
  Impala Public Jenkins: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/11604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibf882428931e4f4264be2fc8cd9d6b1fc89b8ace
Gerrit-Change-Number: 11604
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>

[Impala-ASF-CR] IMPALA-5031: fix signed overflows in decimal

Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/11604 )

Change subject: IMPALA-5031: fix signed overflows in decimal
......................................................................


Patch Set 1:

> Took a first pass on this. Functionally, it makes sense. Is there
 > any performance impact? Does ArithmeticUtil::AsUnsigned() always
 > get inlined?

No observable performance impact - it always gets inlined.


-- 
To view, visit http://gerrit.cloudera.org:8080/11604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf882428931e4f4264be2fc8cd9d6b1fc89b8ace
Gerrit-Change-Number: 11604
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Oct 2018 01:38:56 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: fix signed overflows in decimal

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/11604 )

Change subject: IMPALA-5031: fix signed overflows in decimal
......................................................................


Patch Set 1:

Took a first pass on this. Functionally, it makes sense. Is there any performance impact? Does ArithmeticUtil::AsUnsigned() always get inlined?


-- 
To view, visit http://gerrit.cloudera.org:8080/11604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf882428931e4f4264be2fc8cd9d6b1fc89b8ace
Gerrit-Change-Number: 11604
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Oct 2018 22:09:00 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: fix signed overflows in decimal

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11604 )

Change subject: IMPALA-5031: fix signed overflows in decimal
......................................................................


Patch Set 2:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3309/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/11604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf882428931e4f4264be2fc8cd9d6b1fc89b8ace
Gerrit-Change-Number: 11604
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Comment-Date: Sat, 13 Oct 2018 02:29:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: fix signed overflows in decimal

Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/11604 )

Change subject: IMPALA-5031: fix signed overflows in decimal
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11604/3/be/src/util/decimal-util.h
File be/src/util/decimal-util.h:

http://gerrit.cloudera.org:8080/#/c/11604/3/be/src/util/decimal-util.h@22
PS3, Line 22: #include <functional>
> Why is this required here?  Shouldn't it be required by arithmetic-util.h a
Hi Michal. This code review is already closed and submitted; It's OK to chat here, I just wanted to make it clear that this is already in.

Maybe, but since arithmetic-util.h doesn't technically use anything from <functional>, it's in a bit of a weird spot, as often happens with duck-typed templates. I think it would fail an iwyu check, for instance.



-- 
To view, visit http://gerrit.cloudera.org:8080/11604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf882428931e4f4264be2fc8cd9d6b1fc89b8ace
Gerrit-Change-Number: 11604
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 18:14:36 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5031: fix signed overflows in decimal

Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/11604 )

Change subject: IMPALA-5031: fix signed overflows in decimal
......................................................................


Patch Set 2: Code-Review+2

Carry Joe's +2


-- 
To view, visit http://gerrit.cloudera.org:8080/11604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf882428931e4f4264be2fc8cd9d6b1fc89b8ace
Gerrit-Change-Number: 11604
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Comment-Date: Sat, 13 Oct 2018 02:28:56 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: fix signed overflows in decimal

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11604 )

Change subject: IMPALA-5031: fix signed overflows in decimal
......................................................................


Patch Set 2: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/11604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf882428931e4f4264be2fc8cd9d6b1fc89b8ace
Gerrit-Change-Number: 11604
Gerrit-PatchSet: 2
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Comment-Date: Sat, 13 Oct 2018 06:23:16 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5031: fix signed overflows in decimal

Posted by "Michal Ostrowski (Code Review)" <ge...@cloudera.org>.
Michal Ostrowski has posted comments on this change. ( http://gerrit.cloudera.org:8080/11604 )

Change subject: IMPALA-5031: fix signed overflows in decimal
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11604/3/be/src/util/decimal-util.h
File be/src/util/decimal-util.h:

http://gerrit.cloudera.org:8080/#/c/11604/3/be/src/util/decimal-util.h@22
PS3, Line 22: #include <functional>
Why is this required here?  Shouldn't it be required by arithmetic-util.h and included there?



-- 
To view, visit http://gerrit.cloudera.org:8080/11604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf882428931e4f4264be2fc8cd9d6b1fc89b8ace
Gerrit-Change-Number: 11604
Gerrit-PatchSet: 3
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michal Ostrowski <mo...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Oct 2018 16:02:19 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5031: fix signed overflows in decimal

Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/11604 )

Change subject: IMPALA-5031: fix signed overflows in decimal
......................................................................


Patch Set 1: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/11604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibf882428931e4f4264be2fc8cd9d6b1fc89b8ace
Gerrit-Change-Number: 11604
Gerrit-PatchSet: 1
Gerrit-Owner: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Oct 2018 17:26:14 +0000
Gerrit-HasComments: No