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-2020: Add rounding for decimal casts
Date Tue, 21 Feb 2017 18:32:46 GMT
Zach Amsden has posted comments on this change.

Change subject: IMPALA-2020: Add rounding for decimal casts
......................................................................


Patch Set 12:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/5951/15//COMMIT_MSG
Commit Message:

Line 122
> Sure, we can try to get this back in follow on commits.
Will check


http://gerrit.cloudera.org:8080/#/c/5951/15/be/src/benchmarks/expr-benchmark.cc
File be/src/benchmarks/expr-benchmark.cc:

PS15, Line 218:   BENCHMARK("strncmp1", "'abcdefghi
> Can you please add a sample output like other benchmark functions ?
I can't - with these changes, this is in a slightly better state, but the benchmark still
doesn't run, it segfaults almost immediately due to a NULL descriptor table.  After spending
the better part of a day attempting to fix it, I've had to shelve this for now.  Considering
that there are serious issues running this through codegen anyway, I'm not sure what to do
with these changes.


http://gerrit.cloudera.org:8080/#/c/5951/15/be/src/runtime/decimal-test.cc
File be/src/runtime/decimal-test.cc:

PS15, Line 169: -.1
> -0.1.
Done


PS15, Line 224: Decimal4Value::FromDouble(t4, 0.499999999, true, &overflow);
> Isn't a duplicate of the test above ?
This was supposed to be negative :)


http://gerrit.cloudera.org:8080/#/c/5951/15/be/src/runtime/decimal-value.h
File be/src/runtime/decimal-value.h:

PS15, Line 52: truncating digits that
             :   /// cannot be represented or rounding if desired
> rounding to the closest integer if 'round' is true. Truncate the decimal pl
Done


http://gerrit.cloudera.org:8080/#/c/5951/10/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:

PS10, Line 38:   if (round) {
             :     // Decimal V2 behavior
             : 
             :     // Multiply the double by the scale.
             :     //
             :     // Some quick research shows us that 10**38 in binary has exactly 53
             :     // significant nonzero bits.  Coincidentally, double precision floating
             :     // point gives us 53 bits for the mantissa.  TODO: validate that the
             :     // values produced by the template are both correct, constant and that
             :     // the multiply does not lose precision.
             : 
             :     d *= DecimalUtil::GetScaleMultiplier<double>(scale);
             :     d = std::round(d);
             :     const T max_value = DecimalUtil::GetScaleMultiplier<T>(precision);
             :     if (max_value < 0 || UNLIKELY(std::isnan(d)) || UNLIKELY(std::abs(d)
>= max_value)) {
             :       *overflow = true;
             :       return DecimalValue();
             :     }
             : 
             :     // Return the rounded integer part.
             :     return DecimalValue(static_cast<T>(d));
             :   } else {
             :     // TODO: IMPALA-4924: remove DECIMAL V1 code
             : 
             :     // Check overflow.
             :     const T max_value = DecimalUtil::GetScaleMultiplier<T>(precision
- scale);
             :     if (abs(d) >= max_value) {
             :       *overflow = true;
             :       return DecimalValue();
             :     }
             : 
             :     // Multiply the double by the scale.
             :     d *= DecimalUtil::GetScaleMultiplier<double>(scale);
             : 
             :    
> Are there particular cases you have in mind which may break if we use V2's 
None I can come up with right now, but this was before I noticed the bit pattern for 10^37
fits exactly in a a double.  Now I'm reasonably sure it is safe.


http://gerrit.cloudera.org:8080/#/c/5951/12/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:

PS12, Line 52: max_value < 0
> Yes, if it's not possible (i.e. only happens due to a software bug somewher
Done


http://gerrit.cloudera.org:8080/#/c/5951/15/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:

PS15, Line 132: 
> nit: long line
fixed


-- 
To view, visit http://gerrit.cloudera.org:8080/5951
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2daf186b4770a022f9cb349d512067a1dd624810
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <zamsden@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message