Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17303 )
Change subject: IMPALA-10350 Fix precision loss while converting DecimalValue to double. ...................................................................... Patch Set 1: (30 comments) http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/runtime/decimal-value.inline.h File be/src/runtime/decimal-value.inline.h: http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/runtime/decimal-value.inline.h@727 PS1, Line 727: // Original approach was to use: line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/runtime/decimal-value.inline.h@728 PS1, Line 728: // static_cast<double>(value_) / pow(10.0, scale). line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/runtime/decimal-value.inline.h@729 PS1, Line 729: // However only integers from −2^53 to 2^53 can be represented accurately by line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/runtime/decimal-value.inline.h@730 PS1, Line 730: // double precision without any loss. line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/runtime/decimal-value.inline.h@731 PS1, Line 731: // Hence, it would not work for numbers like -0.43149576573887316. line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/runtime/decimal-value.inline.h@734 PS1, Line 734: // not be accurate. In newer approach we are using line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/runtime/decimal-value.inline.h@742 PS1, Line 742: // It expects value to be positive even for negative numbers. line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/runtime/decimal-value.inline.h@744 PS1, Line 744: double result = fast_double_parser::compute_float_64((-scale), line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/runtime/decimal-value.inline.h@747 PS1, Line 747: // Fallback to original approach. This is not always accurate as described above. line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/runtime/decimal-value.inline.h@749 PS1, Line 749: // double using std:strtod. However std::strtod is atleast 4X slower line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h File be/src/thirdparty/fast_double_parser/fast_double_parser.h: http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@13 PS1, Line 13: #if (defined(sun) || defined(__sun)) line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@17 PS1, Line 17: #if defined(__CYGWIN__) || defined(__MINGW32__) || defined(__MINGW64__) line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@22 PS1, Line 22: * Determining whether we should import xlocale.h or not is line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@25 PS1, Line 25: #if defined(FAST_DOUBLE_PARSER_SOLARIS) || defined(FAST_DOUBLE_PARSER_CYGWIN) line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@67 PS1, Line 67: #endif // defined(FAST_DOUBLE_PARSER_SOLARIS) || defined(FAST_DOUBLE_PARSER_CYGWIN) line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@84 PS1, Line 84: * However, we have that line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@86 PS1, Line 86: * Thus it is possible for a number of the form w * 10^-342 where line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@94 PS1, Line 94: * Any number of form w * 10^309 where w>= 1 is going to be line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@153 PS1, Line 153: // credit: https://stackoverflow.com/questions/28868367/getting-the-high-part-of-64-bit-integer-multiplication line too long (110 > 90) http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@154 PS1, Line 154: really_inline uint64_t Emulate64x64to128(uint64_t& r_hi, const uint64_t x, const uint64_t y) { line too long (94 > 90) http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@159 PS1, Line 159: line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@224 PS1, Line 224: */ line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@958 PS1, Line 958: line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@960 PS1, Line 960: // The exponent is 1024 + 63 + power line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@976 PS1, Line 976: // The 65536 is (1<<16) and corresponds to line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@979 PS1, Line 979: // ((152170 * power ) >> 16) is equal to line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@980 PS1, Line 980: // floor(log(5**power)/log(2)) line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@982 PS1, Line 982: // Note that this is not magic: 152170/(1<<16) is line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@984 PS1, Line 984: // The 1<<16 value is a power of two; we could use a line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/17303/1/be/src/thirdparty/fast_double_parser/fast_double_parser.h@1097 PS1, Line 1097: #if defined(FAST_DOUBLE_PARSER_SOLARIS) || defined(FAST_DOUBLE_PARSER_CYGWIN) line has trailing whitespace -- To view, visit http://gerrit.cloudera.org:8080/17303 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I56f0652cb8f81a491b87d9b108a94c00ae6c99a1 Gerrit-Change-Number: 17303 Gerrit-PatchSet: 1 Gerrit-Owner: Amogh Margoor <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Sun, 11 Apr 2021 17:39:54 +0000 Gerrit-HasComments: Yes
