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: Inline big number strings
Date Fri, 03 Mar 2017 01:34:35 GMT
Zach Amsden has posted comments on this change.

Change subject: IMPALA-2020: Inline big number strings
......................................................................


Patch Set 5:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/5902/5//COMMIT_MSG
Commit Message:

PS5, Line 38: 0.83s
> I mean run the test a few times and provide something like https://en.wikip
Do we have any nice scripts to do that?  I'd rather not go to that trouble when all that is
left is the obvious win of getting the constants declared in the header and not requiring
DecimalUtil::InitMaxUnscaledDecimal()


http://gerrit.cloudera.org:8080/#/c/5902/5/be/src/util/decimal-util.h
File be/src/util/decimal-util.h:

Line 57:   template<typename T>
> Neither looks done in PS9: private or rename
Moot point now - killed the code


PS5, Line 58: T
> I'm not so sure: it would lose precision there in the low bits.
Sadly, other uses GetScaleMultiplier below for exactly this - to get a power of 10 in double.


PS5, Line 59: boost::
> It helps me keep track of comments on patches when they are responded to on
Duly noted


Line 60:     return scale == 0 ? 1 : 10 * GetConstScaleMultiplier<T>(scale-1);
> Why not just another '?:' operator?
moot


Line 136:   if (__builtin_const_p(scale) && p < 10) return GetConstScaleMultiplier<int32_t>(scale);
> Please post the numbers here when you have decided.
Wash.  The only win was from getting the MAX_UNSCALED_DECIMAL constants inline.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5095a366d914cebb0b64bd434a08dbb55c90ed30
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <zamsden@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Zach Amsden <zamsden@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message