Taras Bobrovytsky has posted comments on this change. ( http://gerrit.cloudera.org:8080/8774 )
Change subject: IMPALA-5014: Part 1: Round when casting string to decimal ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/8774/2/be/src/runtime/decimal-test.cc File be/src/runtime/decimal-test.cc: http://gerrit.cloudera.org:8080/#/c/8774/2/be/src/runtime/decimal-test.cc@302 PS2, Line 302: StringToAllDecimals("0.5", 5, 0, > More interesting test cases: Done http://gerrit.cloudera.org:8080/#/c/8774/2/be/src/util/string-parser.h File be/src/util/string-parser.h: http://gerrit.cloudera.org:8080/#/c/8774/2/be/src/util/string-parser.h@191 PS2, Line 191: } else if (UNLIKELY(round && total_digits_count == type_precision)) { > This doesn't need to be conditional on round, in fact I think leaving that We are saving the first truncated digit here. We save the truncated digit in order to be able to round if necessary. If round is false, there is no point in saving the first truncated digit. Why do you think it's better for it to not be conditional on round? Also, which DCHECK are you talking about? the one on line 194? http://gerrit.cloudera.org:8080/#/c/8774/2/be/src/util/string-parser.h@238 PS2, Line 238: // There are less than maximum number of digits to the left of the dot. > Don't you mean "There are more than the maximum number of digits to the rig We already know that there are too many digits to the right of the dot at this point because of comment on line 232. (which means we will be doing rounding for sure if round=true) We are now checking on line 237 what's going on to the left of the dot. In this case, the number looks like this: x.xxxx with let's say prec=3, scale=1. If the number looked like this xx.xxxx, the condition on line 237 would be false. (In other words, we are checking if the part of the number to the left of the dot is "full" or not on line 237). If it's full, then we need to look at the first truncated digit. Otherwise, we can round without having to look at the first truncated digit. http://gerrit.cloudera.org:8080/#/c/8774/2/be/src/util/string-parser.h@239 PS2, Line 239: value = DecimalUtil::ScaleDownAndRound<T>(value, shift, round); > After thinking on this some more, rounding away from zero is symmetric with Agreed. No changes made. http://gerrit.cloudera.org:8080/#/c/8774/2/be/src/util/string-parser.h@243 PS2, Line 243: // There are a maximum number of digits to the left of the dot. We attempt > to the right of the dot? No, to the left. See comment on line 238. http://gerrit.cloudera.org:8080/#/c/8774/2/be/src/util/string-parser.h@248 PS2, Line 248: DCHECK(first_truncated_digit == 0 || round); > I found this DCHECK confusing until I realized saving the truncated digit w It's still not clear to me why it shouldn't be conditional on round. Look at my comment above. http://gerrit.cloudera.org:8080/#/c/8774/2/be/src/util/string-parser.h@250 PS2, Line 250: value += (first_truncated_digit >= 5); > This rounds towards positive infinity which isn't consistent with our round Correct. We negate at the very bottom on line 266. So we are rounding away from zero. http://gerrit.cloudera.org:8080/#/c/8774/2/be/src/util/string-parser.h@262 PS2, Line 262: DCHECK_EQ(truncated_digit_count, 0); > It would be nice to have a DCHECK that this can't overflow. While that is Done http://gerrit.cloudera.org:8080/#/c/8774/2/be/src/util/string-parser.h@391 PS2, Line 391: int digits_after_dot_count, int type_precision, int8_t exponent, T* value, > type_precision is unused in this function Nice catch, removed. -- To view, visit http://gerrit.cloudera.org:8080/8774 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icd8b92727fb384e6ff2d145e4aab7ae5d27db26d Gerrit-Change-Number: 8774 Gerrit-PatchSet: 2 Gerrit-Owner: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@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: Fri, 15 Dec 2017 22:09:56 +0000 Gerrit-HasComments: Yes
