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<RESULT_T>(); : } 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 <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-Comment-Date: Mon, 23 Oct 2017 20:15:44 +0000 Gerrit-HasComments: Yes
