Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8309 )
Change subject: IMPALA-5019: Decimal V2 addition ...................................................................... Patch Set 2: (12 comments) I spent some time getting to understand the code. Still don't 100% comprehend it but getting closer. 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? 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 like MinLeadingZerosAfterScaling() 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, right? I found this notation a bit confusing. 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 they're constants. I'm not sure that templates are really necessary here though. It seems like we could just factor out the shared parts into helper functions and have different functions implementing the same sign/different sign algorithms. E.g. SeparateFractional(x, y, x_scale, y_scale, &x_left, &x_right, &y_left, &y_right); int max_scale = std::max(x_scale, y_scale); int delta_scale = max_scale - result_scale; ... do the same/different sign algorithm ... ... 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 the caller, right? Do we test cases with multiplying by 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 new to the code. 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 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) and (delta_scale > 0) - it's less obvious this way that it's forcing delta_scale to be positive. http://gerrit.cloudera.org:8080/#/c/8309/2/be/src/runtime/decimal-value.inline.h@299 PS2, Line 299: into long line. 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 AdjustToSameScale() - it seems like the delta_scale logic in this function and AdjustToSameScale() are tightly coupled - AdjustToSameScale() doesn't really abstract it away if the caller has to know the details to correctly adjust the output result. I'm not sure if there's a better way to factor it. 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 there's some way we can improve how AdjustToSameScale() is factored so that it's clearer what the expected result scale of that is). 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. -- 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 <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-Comment-Date: Thu, 26 Oct 2017 00:18:26 +0000 Gerrit-HasComments: Yes
