impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5014: Part 1: Round when casting string to decimal
Date Wed, 13 Dec 2017 00:35:22 GMT
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 <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Dec 2017 00:35:22 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message