[Impala-ASF-CR] IMPALA-10350 Fix precision loss while converting DecimalValue to double.
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 4: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/8554/ : Initial code review checks failed. See linked job for details on the failure. -- 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: 4 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Zoltan Borok-Nagy Gerrit-Comment-Date: Mon, 12 Apr 2021 19:18:11 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10350 Fix precision loss while converting DecimalValue to double.
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 4:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/17303/4/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/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@13
PS4, Line 13: #if (defined(sun) || defined(__sun))
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@17
PS4, Line 17: #if defined(__CYGWIN__) || defined(__MINGW32__) ||
defined(__MINGW64__)
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@22
PS4, Line 22: * Determining whether we should import xlocale.h or not is
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@25
PS4, Line 25: #if defined(FAST_DOUBLE_PARSER_SOLARIS) ||
defined(FAST_DOUBLE_PARSER_CYGWIN)
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@67
PS4, Line 67: #endif // defined(FAST_DOUBLE_PARSER_SOLARIS) ||
defined(FAST_DOUBLE_PARSER_CYGWIN)
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@84
PS4, Line 84: * However, we have that
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@86
PS4, 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/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@94
PS4, 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/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@153
PS4, 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/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@154
PS4, 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/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@159
PS4, Line 159:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@224
PS4, Line 224: */
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@958
PS4, Line 958:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@960
PS4, Line 960: // The exponent is 1024 + 63 + power
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@976
PS4, Line 976: // The 65536 is (1<<16) and corresponds to
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@979
PS4, Line 979: // ((152170 * power ) >> 16) is equal to
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@980
PS4, Line 980: // floor(log(5**power)/log(2))
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@982
PS4, Line 982: // Note that this is not magic: 152170/(1<<16) is
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@984
PS4, 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/4/be/src/thirdparty/fast_double_parser/fast_double_parser.h@1097
PS4, 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: 4
Gerrit-Owner: Amogh Margoor
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Mon, 12 Apr 2021 18:59:23 +
Gerrit-HasCom
[Impala-ASF-CR] IMPALA-10350 Fix precision loss while converting DecimalValue to double.
Amogh Margoor has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/17303 ) Change subject: IMPALA-10350 Fix precision loss while converting DecimalValue to double. .. IMPALA-10350 Fix precision loss while converting DecimalValue to double. Original approach to convert DecimalValue(internal representation of decimals) was not accurate. It was: static_cast(value_) / pow(10.0, scale). However only integers from −2^53 to 2^53 can be represented accurately by double precision without any loss. Hence, it would not work for numbers like -0.43149576573887316. For DecimalValue representing -0.43149576573887316, value_ would be -43149576573887316 and scale would be 17. As value_ < -2^53, result would not be accurate. In newer approach we are using third party library https://github.com/lemire/fast_double_parser, which handles above scenario in a performant manner. Change-Id: I56f0652cb8f81a491b87d9b108a94c00ae6c99a1 --- M be/src/runtime/decimal-value.inline.h A be/src/thirdparty/fast_double_parser/LICENSE A be/src/thirdparty/fast_double_parser/LICENSE.BSL A be/src/thirdparty/fast_double_parser/README.md A be/src/thirdparty/fast_double_parser/fast_double_parser.h M bin/rat_exclude_files.txt M testdata/workloads/functional-query/queries/QueryTest/values.test 7 files changed, 1,551 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/17303/4 -- 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: newpatchset Gerrit-Change-Id: I56f0652cb8f81a491b87d9b108a94c00ae6c99a1 Gerrit-Change-Number: 17303 Gerrit-PatchSet: 4 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Zoltan Borok-Nagy
[Impala-ASF-CR] IMPALA-10350 Fix precision loss while converting DecimalValue to double.
Zoltan Borok-Nagy 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 3:
(7 comments)
The change looks great overall!
http://gerrit.cloudera.org:8080/#/c/17303/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17303/3//COMMIT_MSG@7
PS3, Line 7: IMPALA-10350
Please add : after IMPALA-10350
http://gerrit.cloudera.org:8080/#/c/17303/3//COMMIT_MSG@9
PS3, Line 9: imals) was not accurate.
In commit message bodies we use lines with 72 chars width.
http://gerrit.cloudera.org:8080/#/c/17303/3//COMMIT_MSG@20
PS3, Line 20:
Please add section about testing.
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/runtime/decimal-value.inline.h@727
PS3, Line 727: // Original approach was to use:
Thanks for the detailed comments!
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/runtime/decimal-value.inline.h@745
PS3, Line 745: int64_t> (value_
Type of value_ can be __int128_t. If that's the case and the value is greater
than the max value of int64_t, then I think we also want to fallback to the
original algorithm.
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/runtime/decimal-value.inline.h@747
PS3, Line 747: // Fallback to original approach. This is not always accurate
as described above.
: // Other alternative would be to convert value_ to string and
parse it into
: // double using std:strtod. However std::strtod is atleast 4X
slower
: // than this approach
(https://github.com/lemire/fast_double_parser#sample-results)
: // and that is excluding cost to convert value_ to string.
: if (!success) {
: result = static_cast(value_) / pow(10.0, scale);
: }
I'm happy with the current patch as it is a big step forward, but IMPALA-10350
is about correctness and I think we still want to track that problem in Jira.
How about creating a sub-task for IMPALA-10350 and commit this patch with that
Jira ID? We'll close the sub-task once this patch is committed, but leave
IMPALA-10350 open so we'll be aware of the limitations. Hopefully we'll find a
proper algorithm in the future. (If this patch was using strtod then we'd still
need another Jira to track perf improvements possibilities in the future).
http://gerrit.cloudera.org:8080/#/c/17303/3/testdata/workloads/functional-query/queries/QueryTest/values.test
File testdata/workloads/functional-query/queries/QueryTest/values.test:
http://gerrit.cloudera.org:8080/#/c/17303/3/testdata/workloads/functional-query/queries/QueryTest/values.test@106
PS3, Line 106: # insert one negative value < -2^53 * 10^(-17)
Please add test where the value needs to be stored in __int128_t.
--
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: 3
Gerrit-Owner: Amogh Margoor
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Zoltan Borok-Nagy
Gerrit-Comment-Date: Mon, 12 Apr 2021 15:50:54 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10350 Fix precision loss while converting DecimalValue to double.
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 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/8544/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 3 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Sun, 11 Apr 2021 20:28:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10350 Fix precision loss while converting DecimalValue to double.
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 3:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/17303/3/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/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@13
PS3, Line 13: #if (defined(sun) || defined(__sun))
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@17
PS3, Line 17: #if defined(__CYGWIN__) || defined(__MINGW32__) ||
defined(__MINGW64__)
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@22
PS3, Line 22: * Determining whether we should import xlocale.h or not is
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@25
PS3, Line 25: #if defined(FAST_DOUBLE_PARSER_SOLARIS) ||
defined(FAST_DOUBLE_PARSER_CYGWIN)
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@67
PS3, Line 67: #endif // defined(FAST_DOUBLE_PARSER_SOLARIS) ||
defined(FAST_DOUBLE_PARSER_CYGWIN)
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@84
PS3, Line 84: * However, we have that
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@86
PS3, 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/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@94
PS3, 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/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@153
PS3, 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/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@154
PS3, 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/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@159
PS3, Line 159:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@224
PS3, Line 224: */
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@958
PS3, Line 958:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@960
PS3, Line 960: // The exponent is 1024 + 63 + power
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@976
PS3, Line 976: // The 65536 is (1<<16) and corresponds to
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@979
PS3, Line 979: // ((152170 * power ) >> 16) is equal to
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@980
PS3, Line 980: // floor(log(5**power)/log(2))
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@982
PS3, Line 982: // Note that this is not magic: 152170/(1<<16) is
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@984
PS3, 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/3/be/src/thirdparty/fast_double_parser/fast_double_parser.h@1097
PS3, 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: 3
Gerrit-Owner: Amogh Margoor
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Sun, 11 Apr 2021 20:08:27 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10350 Fix precision loss while converting DecimalValue to double.
Amogh Margoor has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/17303 ) Change subject: IMPALA-10350 Fix precision loss while converting DecimalValue to double. .. IMPALA-10350 Fix precision loss while converting DecimalValue to double. Original approach to convert DecimalValue(internal representation of decimals) was not accurate. It was: static_cast(value_) / pow(10.0, scale). However only integers from −2^53 to 2^53 can be represented accurately by double precision without any loss. Hence, it would not work for numbers like -0.43149576573887316. For DecimalValue representing -0.43149576573887316, value_ would be -43149576573887316 and scale would be 17. As value_ < -2^53, result would not be accurate. In newer approach we are using third party library https://github.com/lemire/fast_double_parser, which handles above scenario in a performant manner. Change-Id: I56f0652cb8f81a491b87d9b108a94c00ae6c99a1 --- M be/src/runtime/decimal-value.inline.h A be/src/thirdparty/fast_double_parser/LICENSE A be/src/thirdparty/fast_double_parser/LICENSE.BSL A be/src/thirdparty/fast_double_parser/README.md A be/src/thirdparty/fast_double_parser/fast_double_parser.h M bin/rat_exclude_files.txt M testdata/workloads/functional-query/queries/QueryTest/values.test 7 files changed, 1,541 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/17303/3 -- 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: newpatchset Gerrit-Change-Id: I56f0652cb8f81a491b87d9b108a94c00ae6c99a1 Gerrit-Change-Number: 17303 Gerrit-PatchSet: 3 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-10350 Fix precision loss while converting DecimalValue to double.
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 2: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/8543/ : Initial code review checks failed. See linked job for details on the failure. -- 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: 2 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Sun, 11 Apr 2021 18:39:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10350 Fix precision loss while converting DecimalValue to double.
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 2:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/17303/2/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/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@13
PS2, Line 13: #if (defined(sun) || defined(__sun))
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@17
PS2, Line 17: #if defined(__CYGWIN__) || defined(__MINGW32__) ||
defined(__MINGW64__)
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@22
PS2, Line 22: * Determining whether we should import xlocale.h or not is
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@25
PS2, Line 25: #if defined(FAST_DOUBLE_PARSER_SOLARIS) ||
defined(FAST_DOUBLE_PARSER_CYGWIN)
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@67
PS2, Line 67: #endif // defined(FAST_DOUBLE_PARSER_SOLARIS) ||
defined(FAST_DOUBLE_PARSER_CYGWIN)
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@84
PS2, Line 84: * However, we have that
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@86
PS2, 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/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@94
PS2, 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/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@153
PS2, 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/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@154
PS2, 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/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@159
PS2, Line 159:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@224
PS2, Line 224: */
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@958
PS2, Line 958:
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@960
PS2, Line 960: // The exponent is 1024 + 63 + power
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@976
PS2, Line 976: // The 65536 is (1<<16) and corresponds to
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@979
PS2, Line 979: // ((152170 * power ) >> 16) is equal to
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@980
PS2, Line 980: // floor(log(5**power)/log(2))
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@982
PS2, Line 982: // Note that this is not magic: 152170/(1<<16) is
line has trailing whitespace
http://gerrit.cloudera.org:8080/#/c/17303/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@984
PS2, 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/2/be/src/thirdparty/fast_double_parser/fast_double_parser.h@1097
PS2, 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: 2
Gerrit-Owner: Amogh Margoor
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Sun, 11 Apr 2021 18:19:27 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10350 Fix precision loss while converting DecimalValue to double.
Amogh Margoor has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/17303 ) Change subject: IMPALA-10350 Fix precision loss while converting DecimalValue to double. .. IMPALA-10350 Fix precision loss while converting DecimalValue to double. Original approach to convert DecimalValue(internal representation of decimals) was not accurate. It was: static_cast(value_) / pow(10.0, scale). However only integers from −2^53 to 2^53 can be represented accurately by double precision without any loss. Hence, it would not work for numbers like -0.43149576573887316. For DecimalValue representing -0.43149576573887316, value_ would be -43149576573887316 and scale would be 17. As value_ < -2^53, result would not be accurate. In newer approach we are using third party library https://github.com/lemire/fast_double_parser, which handles above scenario in a performant manner. Change-Id: I56f0652cb8f81a491b87d9b108a94c00ae6c99a1 --- M be/src/runtime/decimal-value.inline.h A be/src/thirdparty/fast_double_parser/LICENSE A be/src/thirdparty/fast_double_parser/LICENSE.BSL A be/src/thirdparty/fast_double_parser/README.md A be/src/thirdparty/fast_double_parser/fast_double_parser.h M testdata/workloads/functional-query/queries/QueryTest/values.test 6 files changed, 1,540 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/17303/2 -- 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: newpatchset Gerrit-Change-Id: I56f0652cb8f81a491b87d9b108a94c00ae6c99a1 Gerrit-Change-Number: 17303 Gerrit-PatchSet: 2 Gerrit-Owner: Amogh Margoor Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-10350 Fix precision loss while converting DecimalValue to double.
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: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/8542/ : Initial code review checks failed. See linked job for details on the failure. -- 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 Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Sun, 11 Apr 2021 17:58:25 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10350 Fix precision loss while converting DecimalValue to double.
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(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
[Impala-ASF-CR] IMPALA-10350 Fix precision loss while converting DecimalValue to double.
Amogh Margoor has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17303 Change subject: IMPALA-10350 Fix precision loss while converting DecimalValue to double. .. IMPALA-10350 Fix precision loss while converting DecimalValue to double. Original approach to convert DecimalValue(internal representation of decimals) was not accurate. It was: static_cast(value_) / pow(10.0, scale). However only integers from −2^53 to 2^53 can be represented accurately by double precision without any loss. Hence, it would not work for numbers like -0.43149576573887316. For DecimalValue representing -0.43149576573887316, value_ would be -43149576573887316 and scale would be 17. As value_ < -2^53, result would not be accurate. In newer approach we are using third party library https://github.com/lemire/fast_double_parser, which handles above scenario in a performant manner. Change-Id: I56f0652cb8f81a491b87d9b108a94c00ae6c99a1 --- M be/src/runtime/decimal-value.inline.h A be/src/thirdparty/fast_double_parser/LICENSE A be/src/thirdparty/fast_double_parser/LICENSE.BSL A be/src/thirdparty/fast_double_parser/README.md A be/src/thirdparty/fast_double_parser/fast_double_parser.h M testdata/workloads/functional-query/queries/QueryTest/values.test 6 files changed, 1,540 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/17303/1 -- 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: newchange Gerrit-Change-Id: I56f0652cb8f81a491b87d9b108a94c00ae6c99a1 Gerrit-Change-Number: 17303 Gerrit-PatchSet: 1 Gerrit-Owner: Amogh Margoor
