impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <>
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. ( )

Change subject: IMPALA-5014: Part 1: Round when casting string to decimal

Patch Set 2:

File be/src/runtime/
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
File be/src/util/string-parser.h:
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.
PS2, Line 238:         // There are less than maximum number of digits to the left of the
Don't you mean "There are more than the maximum number of digits to the right of the dot."
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.
PS2, Line 243:         // There are a maximum number of digits to the left of the dot. We
to the right of the dot?
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.
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.
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.
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
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Reviewer: Zach Amsden <>
Gerrit-Comment-Date: Wed, 13 Dec 2017 00:35:22 +0000
Gerrit-HasComments: Yes

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