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<double>(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 <amarg...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Mon, 12 Apr 2021 15:50:54 +0000 Gerrit-HasComments: Yes
