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<RESULT_T>(); : } > 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 <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-Comment-Date: Mon, 23 Oct 2017 23:19:35 +0000 Gerrit-HasComments: Yes
