You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2018/01/03 19:16:24 UTC
[Impala-ASF-CR] IMPALA-6355: fix overflow DCHECK in decimal mod
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8929
Change subject: IMPALA-6355: fix overflow DCHECK in decimal mod
......................................................................
IMPALA-6355: fix overflow DCHECK in decimal mod
The bug is that ConvertToInt128(), by design, only sets 'overflow' if an
overflow occurs. This means that caller needs to initialise the overflow
variable to false, otherwise the value when overflow occurs is
undefined.
Testing:
Reprod the expr-test failure under ASAN, confirmed that it passed after
this fix.
Change-Id: Ifd7ac691155442ba7cba71dd3647208b7c1c0bf9
---
M be/src/runtime/decimal-value.inline.h
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/8929/1
--
To view, visit http://gerrit.cloudera.org:8080/8929
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifd7ac691155442ba7cba71dd3647208b7c1c0bf9
Gerrit-Change-Number: 8929
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6355: fix overflow DCHECK in decimal mod
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8929 )
Change subject: IMPALA-6355: fix overflow DCHECK in decimal mod
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8929/1/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:
http://gerrit.cloudera.org:8080/#/c/8929/1/be/src/runtime/decimal-value.inline.h@571
PS1, Line 571: DCHECK(!ovf)
is overflow here a valid case? if yes, should we return it in '*overflow' instead of DCHECKing ?
--
To view, visit http://gerrit.cloudera.org:8080/8929
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd7ac691155442ba7cba71dd3647208b7c1c0bf9
Gerrit-Change-Number: 8929
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Jan 2018 20:11:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6355: fix overflow DCHECK in decimal mod
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8929 )
Change subject: IMPALA-6355: fix overflow DCHECK in decimal mod
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8929/1/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:
http://gerrit.cloudera.org:8080/#/c/8929/1/be/src/runtime/decimal-value.inline.h@571
PS1, Line 571: DCHECK(!ovf)
> is overflow here a valid case? if yes, should we return it in '*overflow' i
It shouldn't be. I believe that the properties of decimal mod and the way the planner chooses the output precision/scale guarantee that the result should always fit in the output type
--
To view, visit http://gerrit.cloudera.org:8080/8929
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd7ac691155442ba7cba71dd3647208b7c1c0bf9
Gerrit-Change-Number: 8929
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Jan 2018 20:27:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6355: fix overflow DCHECK in decimal mod
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8929 )
Change subject: IMPALA-6355: fix overflow DCHECK in decimal mod
......................................................................
Patch Set 1: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/8929
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd7ac691155442ba7cba71dd3647208b7c1c0bf9
Gerrit-Change-Number: 8929
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 04 Jan 2018 02:30:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6355: fix overflow DCHECK in decimal mod
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/8929 )
Change subject: IMPALA-6355: fix overflow DCHECK in decimal mod
......................................................................
Patch Set 1: Code-Review+2
I feel comfortable giving +2 here alongside Bikram's +1: I had independently seen the DCHECK was due to !ovf; this fix seems straightforward. Thanks for doing so.
--
To view, visit http://gerrit.cloudera.org:8080/8929
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd7ac691155442ba7cba71dd3647208b7c1c0bf9
Gerrit-Change-Number: 8929
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Jan 2018 23:11:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6355: fix overflow DCHECK in decimal mod
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8929 )
Change subject: IMPALA-6355: fix overflow DCHECK in decimal mod
......................................................................
Patch Set 1:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1672/
--
To view, visit http://gerrit.cloudera.org:8080/8929
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd7ac691155442ba7cba71dd3647208b7c1c0bf9
Gerrit-Change-Number: 8929
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Jan 2018 22:52:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6355: fix overflow DCHECK in decimal mod
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/8929 )
Change subject: IMPALA-6355: fix overflow DCHECK in decimal mod
......................................................................
IMPALA-6355: fix overflow DCHECK in decimal mod
The bug is that ConvertToInt128(), by design, only sets 'overflow' if an
overflow occurs. This means that caller needs to initialise the overflow
variable to false, otherwise the value when overflow occurs is
undefined.
Testing:
Reprod the expr-test failure under ASAN, confirmed that it passed after
this fix.
Change-Id: Ifd7ac691155442ba7cba71dd3647208b7c1c0bf9
Reviewed-on: http://gerrit.cloudera.org:8080/8929
Reviewed-by: Bikramjeet Vig <bi...@cloudera.com>
Reviewed-by: Michael Brown <mi...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/runtime/decimal-value.inline.h
1 file changed, 1 insertion(+), 1 deletion(-)
Approvals:
Bikramjeet Vig: Looks good to me, but someone else must approve
Michael Brown: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8929
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifd7ac691155442ba7cba71dd3647208b7c1c0bf9
Gerrit-Change-Number: 8929
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-6355: fix overflow DCHECK in decimal mod
Posted by "Bikramjeet Vig (Code Review)" <ge...@cloudera.org>.
Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8929 )
Change subject: IMPALA-6355: fix overflow DCHECK in decimal mod
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8929/1/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:
http://gerrit.cloudera.org:8080/#/c/8929/1/be/src/runtime/decimal-value.inline.h@571
PS1, Line 571: DCHECK(!ovf)
> It shouldn't be. I believe that the properties of decimal mod and the way t
I see. Thanks for the clarification!
--
To view, visit http://gerrit.cloudera.org:8080/8929
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd7ac691155442ba7cba71dd3647208b7c1c0bf9
Gerrit-Change-Number: 8929
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Jan 2018 22:41:50 +0000
Gerrit-HasComments: Yes