Zach Amsden 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: 0.00000 as 6,5 1.00000 as 6.5 0.000000 as 6,5 1.000000 as 6,5 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 condition out makes reading the DCHECK below simpler. 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 right of the dot." ? 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); ScaleDownAndRound implements signed rounding to do rounding away from zero, so value should be negated if is_negative is true. 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? 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 was conditional on round, which I don't think it needs to be. 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 rounding behavior in ScaleDownAndRound, which rounds away from zero. Edit: Actually, since the value isn't negated yet, this is correct. Where to actually do the negation appears to be a bit of a conundrum. 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 true because of the structure of the if conditions, there are enough of them to invite uncertainty in the readers mind by the time we get here. 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 -- 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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-Comment-Date: Wed, 13 Dec 2017 00:35:22 +0000 Gerrit-HasComments: Yes
