[Impala-ASF-CR] IMPALA-4964: Fix Decimal modulo overflow
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8329 ) Change subject: IMPALA-4964: Fix Decimal modulo overflow .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd Gerrit-Change-Number: 8329 Gerrit-PatchSet: 3 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Tue, 31 Oct 2017 22:47:37 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4964: Fix Decimal modulo overflow
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8329 ) Change subject: IMPALA-4964: Fix Decimal modulo overflow .. Patch Set 3: I'll do another pass with fresh eyes. I didn't have any specific concerns. -- To view, visit http://gerrit.cloudera.org:8080/8329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd Gerrit-Change-Number: 8329 Gerrit-PatchSet: 3 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Tue, 31 Oct 2017 18:24:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4964: Fix Decimal modulo overflow
Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8329 ) Change subject: IMPALA-4964: Fix Decimal modulo overflow .. Patch Set 3: Tim, do you want a second pair of eyes on this? If not, could you do the +2 review for this? -- To view, visit http://gerrit.cloudera.org:8080/8329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd Gerrit-Change-Number: 8329 Gerrit-PatchSet: 3 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Tue, 31 Oct 2017 18:23:16 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4964: Fix Decimal modulo overflow
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8329 ) Change subject: IMPALA-4964: Fix Decimal modulo overflow .. Patch Set 3: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/8329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd Gerrit-Change-Number: 8329 Gerrit-PatchSet: 3 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Mon, 30 Oct 2017 18:50:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4964: Fix Decimal modulo overflow
Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/8329 ) Change subject: IMPALA-4964: Fix Decimal modulo overflow .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/exprs/expr-test.cc@2286 PS1, Line 2286: // Test modulo operator > It would be helpful to comment that (-x) % y == -(x % y) to make it easier Done http://gerrit.cloudera.org:8080/#/c/8329/2/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: http://gerrit.cloudera.org:8080/#/c/8329/2/be/src/runtime/decimal-value.inline.h@505 PS2, Line 505: // inputs to int256_t. > Maybe briefly mention that we want to avoid int256_t is very slow - it's ob Done -- To view, visit http://gerrit.cloudera.org:8080/8329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd Gerrit-Change-Number: 8329 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Tue, 24 Oct 2017 00:31:57 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4964: Fix Decimal modulo overflow
Taras Bobrovytsky has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/8329 ) Change subject: IMPALA-4964: Fix Decimal modulo overflow .. IMPALA-4964: Fix Decimal modulo overflow The modulo operation between two decimals should never overflow. Before this patch, there would be an overflow if the scale difference between the two decimals was large. We would try to scale up the one with the smaller scale, so that the scales matched, which could result in an overflow. We fix the problem by first checking if the scaled up value would fit into 128 bits by estimating the number of leading zeros in it. If we detect that 128 bits is not enough, we convert both numbers to int256, do the operation, then convert back to 128 bits. Testing: - Added some expr tests that excercise the new code path. Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd --- M be/src/exprs/expr-test.cc M be/src/runtime/decimal-test.cc M be/src/runtime/decimal-value.inline.h 3 files changed, 125 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/8329/3 -- To view, visit http://gerrit.cloudera.org:8080/8329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd Gerrit-Change-Number: 8329 Gerrit-PatchSet: 3 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-Reviewer: anujphadke
[Impala-ASF-CR] IMPALA-4964: Fix Decimal modulo overflow
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8329 ) Change subject: IMPALA-4964: Fix Decimal modulo overflow .. Patch Set 2: (2 comments) Looks good, just a couple of things that could be clarified. http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/exprs/expr-test.cc@2286 PS1, Line 2286: // Test modulo operator It would be helpful to comment that (-x) % y == -(x % y) to make it easier to understand the results (I was initially confused about how negative modulo should be handled). (I found this explanation helpful: https://stackoverflow.com/a/1907723) I find negative numbers on the RHS of the modulo even more confusing so it might help to have a similar clarification. http://gerrit.cloudera.org:8080/#/c/8329/2/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: http://gerrit.cloudera.org:8080/#/c/8329/2/be/src/runtime/decimal-value.inline.h@505 PS2, Line 505: // inputs to int256_t. Maybe briefly mention that we want to avoid int256_t is very slow - it's obvious to us now but might help a future reader of the code. -- To view, visit http://gerrit.cloudera.org:8080/8329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd Gerrit-Change-Number: 8329 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Tue, 24 Oct 2017 00:13:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4964: Fix Decimal modulo overflow
Taras Bobrovytsky has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/8329 ) Change subject: IMPALA-4964: Fix Decimal modulo overflow .. IMPALA-4964: Fix Decimal modulo overflow The modulo operation between two decimals should never overflow. Before this patch, there would be an overflow if the scale difference between the two decimals was large. We would try to scale up the one with the smaller scale, so that the scales matched, which could result in an overflow. We fix the problem by first checking if the scaled up value would fit into 128 bits by estimating the number of leading zeros in it. If we detect that 128 bits is not enough, we convert both numbers to int256, do the operation, then convert back to 128 bits. Testing: - Added some expr tests that excercise the new code path. Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd --- M be/src/exprs/expr-test.cc M be/src/runtime/decimal-test.cc M be/src/runtime/decimal-value.inline.h 3 files changed, 120 insertions(+), 27 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/8329/2 -- To view, visit http://gerrit.cloudera.org:8080/8329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd Gerrit-Change-Number: 8329 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-Reviewer: anujphadke
[Impala-ASF-CR] IMPALA-4964: Fix Decimal modulo overflow
Taras Bobrovytsky has posted comments on this change. (
http://gerrit.cloudera.org:8080/8329 )
Change subject: IMPALA-4964: Fix Decimal modulo overflow
..
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h@a281
PS1, Line 281:
:
> May help to keep this comment.
Done
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h@181
PS1, Line 181: CheckAdjustment
> MinLeadingZero() or some other more meaningful name ?
Done
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h@497
PS1, Line 497: (other.value() == 0)
> nit: parenthesis not needed.
Done
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h@499
PS1, Line 499: // Mod by 0.
> Shouldn't we raise an error on decimal modulo 0 operations like you did in
Erroring on mod 0 is handled in another of my patches. We raise an error in
that patch after we set is_nan in this function.
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h@498
PS1, Line 498: if (UNLIKELY(*is_nan)) {
: // Mod by 0.
: return DecimalValue();
: }
> nit: one liner.
Done
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h@505
PS1, Line 505: if (sizeof(RESULT_T) < 16 ||
: result_precision < 38 ||
: // If the scales are the same, there is no danger in
overflowing due to scaling up.
: this_scale == other_scale ||
: detail::CheckAdjustment(value(), this_scale,
other.value(), other_scale) >= 2) {
> May help to add a comment to explain what this check is trying to achieve.
Added a comment to make it more clear. Added a comment about overflow at the
bottom. Mod() should never overflow.
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h@514
PS1, Line 514: DCHECK(
> DCHECK_LT();
Unfortunately this does not work with int128_t.
--
To view, visit http://gerrit.cloudera.org:8080/8329
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd
Gerrit-Change-Number: 8329
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Taras Bobrovytsky
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zach Amsden
Gerrit-Reviewer: anujphadke
Gerrit-Comment-Date: Mon, 23 Oct 2017 23:19:35 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4964: Fix Decimal modulo overflow
anujphadke has posted comments on this change. ( http://gerrit.cloudera.org:8080/8329 ) Change subject: IMPALA-4964: Fix Decimal modulo overflow .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h@499 PS1, Line 499: // Mod by 0. Shouldn't we raise an error on decimal modulo 0 operations like you did in Impala-5018? -- To view, visit http://gerrit.cloudera.org:8080/8329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd Gerrit-Change-Number: 8329 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Zach Amsden Gerrit-Reviewer: anujphadke Gerrit-Comment-Date: Mon, 23 Oct 2017 21:49:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4964: Fix Decimal modulo overflow
Michael Ho has posted comments on this change. (
http://gerrit.cloudera.org:8080/8329 )
Change subject: IMPALA-4964: Fix Decimal modulo overflow
..
Patch Set 1:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h@a281
PS1, Line 281:
:
May help to keep this comment.
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h@181
PS1, Line 181: CheckAdjustment
MinLeadingZero() or some other more meaningful name ?
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h@497
PS1, Line 497: (other.value() == 0)
nit: parenthesis not needed.
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h@498
PS1, Line 498: if (UNLIKELY(*is_nan)) {
: // Mod by 0.
: return DecimalValue();
: }
nit: one liner.
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h@505
PS1, Line 505: if (sizeof(RESULT_T) < 16 ||
: result_precision < 38 ||
: // If the scales are the same, there is no danger in
overflowing due to scaling up.
: this_scale == other_scale ||
: detail::CheckAdjustment(value(), this_scale,
other.value(), other_scale) >= 2) {
May help to add a comment to explain what this check is trying to achieve. It
seems to be checking for cases in which we need to cast x and y up to int256_t
to avoid overflow due to scaling up.
In particular, may help to document the conditions in which overflow may (or
may not) occur.
http://gerrit.cloudera.org:8080/#/c/8329/1/be/src/runtime/decimal-value.inline.h@514
PS1, Line 514: DCHECK(
DCHECK_LT();
--
To view, visit http://gerrit.cloudera.org:8080/8329
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd
Gerrit-Change-Number: 8329
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Zach Amsden
Gerrit-Comment-Date: Mon, 23 Oct 2017 20:15:44 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4964: Fix Decimal modulo overflow
Taras Bobrovytsky has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8329 Change subject: IMPALA-4964: Fix Decimal modulo overflow .. IMPALA-4964: Fix Decimal modulo overflow The modulo operation between two decimals should never overflow. Before this patch, there would be an overflow if the scale difference between the two decimals was large. We would try to scale up the one with the smaller scale, so that the scales matched, which could result in an overflow. We fix the problem by first checking if the scaled up value would fit into 128 bits by estimating the number of leading zeros in it. If we detect that 128 bits is not enough, we convert both numbers to int256, do the operation, then convert back to 128 bits. Testing: - Added some expr tests that excercise the new code path. Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd --- M be/src/exprs/expr-test.cc M be/src/runtime/decimal-test.cc M be/src/runtime/decimal-value.inline.h 3 files changed, 116 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/8329/1 -- To view, visit http://gerrit.cloudera.org:8080/8329 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I5420201d4440d421e33e443df005cdcc16b8a6cd Gerrit-Change-Number: 8329 Gerrit-PatchSet: 1 Gerrit-Owner: Taras Bobrovytsky
