Jim Apple has posted comments on this change. Change subject: IMPALA-2020: Make it easy to work with big numbers ......................................................................
Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/5902/2//COMMIT_MSG Commit Message: PS2, Line 7: Make it easy to work with big numbers More specifically: allow writing 128-bit literals PS2, Line 10: legal C++ This is a gcc/clang issue, not a C++ one Line 16: __int128 bignines = LargeNum<__int128>(9, 38);` What happens if you pass parameters that make it overflow? http://gerrit.cloudera.org:8080/#/c/5902/2/be/src/util/decimal-util.h File be/src/util/decimal-util.h: Line 44: template<typename T> If this is only supposed to be called at compile-time, all the parameters can be template parameters. Line 45: static constexpr T LargeNum(int digit, int precision, int base = 10) { This is the kind of API that I would foul up by swapping the params. Is there anything that be done to make this more fool-proof? Some ideas: 1. digit should be less than base. 2. digit should never be 0 or negative. 3. The top-level recursive call should always produce a number that doesn't fit in 64 bits and does fit in 128 bits 4. Can it be called on a const char *, perhaps, reducing digit and precision to one parameter? PS2, Line 46: ( > nit: extraneous parens Handle non-positive precision. -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden <zams...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Zach Amsden <zams...@cloudera.com> Gerrit-HasComments: Yes
