Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/8309 )
Change subject: IMPALA-5019: Decimal V2 addition ...................................................................... Patch Set 2: (12 comments) http://gerrit.cloudera.org:8080/#/c/8309/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8309/1//COMMIT_MSG@31 PS1, Line 31: to avoid this by first checking if the result would into int128. > fit? Done http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@166 PS2, Line 166: LeadingZeroAdjustment > I think the name could make it clearer what it's computing. Maybe something Done. I like the new name. http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@170 PS2, Line 170: floor(log2(b)) > The table values are actually floor(log2(10^b)) for i = 0, 1, 2, 3... etc, Yes, exactly. Fixed the comment. http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@181 PS2, Line 181: same_sign > We usually make numeric/bool template arguments upper case to make it clear Separated this large function into 3 smaller ones, as you suggested. http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@185 PS2, Line 185: // This is expected to be handled by the caller. > What about the case when they're zero? That's also meant to be handled by t It wasn't possible for this condition to fail because at least one must be greater than zero for the following reasons: - If they are both zero, we will not be calling this function, because of the leading zero test. - If one is negative and the other is zero, we will invert it on line 327 I modified this check in the new implementation. Yes we do have cases, where we add zero. http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@272 PS2, Line 272: DCHECK_EQ(result_scale, std::max(this_scale, other_scale)); > A brief comment that this is guaranteed by the frontend migh help people ne Done http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@277 PS2, Line 277: DCHECK(!*overflow) << "Cannot overflow unless result is Decimal16Value"; > extra indent here Done http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@287 PS2, Line 287: if (this_scale < other_scale) { > It might be slightly easier to follow if the branches were(delta_scale < 0) Rewrote the code to make it more clear. http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@299 PS2, Line 299: into > long line. Done http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@305 PS2, Line 305: bool ovf = AdjustToSameScale(*this, this_scale, other, other_scale, > It feels a bit weird that we're calculating delta_scale above and inside Ad Delta scale is different in AdjustToSameScale(). There are two delta scales in this patch: between x and y and between max(x_scale, y_scale) and result_scale. What we are doing here and in AddLarge() is first adjust to the same scale, do the addition, then scale down to result_scale. http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@310 PS2, Line 310: if (delta_scale > 0) { > This might need a brief comment to explain why it's necessary (or perhaps t Added comment. http://gerrit.cloudera.org:8080/#/c/8309/2/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java File fe/src/main/java/org/apache/impala/analysis/TypesUtil.java: http://gerrit.cloudera.org:8080/#/c/8309/2/fe/src/main/java/org/apache/impala/analysis/TypesUtil.java@228 PS2, Line 228: * TODO: implement V2 rules for ADD/SUB. > TODO not needed. Done -- To view, visit http://gerrit.cloudera.org:8080/8309 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I401049c56d910eb1546a178c909c923b01239336 Gerrit-Change-Number: 8309 Gerrit-PatchSet: 2 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-Comment-Date: Sat, 28 Oct 2017 00:02:20 +0000 Gerrit-HasComments: Yes
