Amogh Margoor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17389 )
Change subject: IMPALA-10680: Replace StringToFloatInternal using fast_double_parser library ...................................................................... Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/17389/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17389/5//COMMIT_MSG@12 PS5, Line 12: doesn't : sacrifise the accuracy for speed. > Could you please add your measurements here as well? Done. http://gerrit.cloudera.org:8080/#/c/17389/5/be/src/util/string-parser.h File be/src/util/string-parser.h: http://gerrit.cloudera.org:8080/#/c/17389/5/be/src/util/string-parser.h@493 PS5, Line 493: ng decimal > When len is super long, c_str may overflow the stack. Maybe std::string or Thats a good catch. I have changed it to string and code is much simpler now. http://gerrit.cloudera.org:8080/#/c/17389/5/be/src/util/string-parser.h@501 PS5, Line 501: > (s+i) is repeatedly computed in this loop. May precompute it as pos=s+i in 'pos' is not going to be 'loop constant' which can be pulled out as it needs to be incremented along with 'i'. So keeping just 1 counter for loop instead of 2. http://gerrit.cloudera.org:8080/#/c/17389/5/be/src/util/string-parser.h@501 PS5, Line 501: : if (UNLIKELY(s_end == nullptr)) { : // Determine if it is an overflow case : if (UNLIKELY(!std::isfinite(val))) { : *result = PARSE_OVERFLOW; : } else if (UNLIKELY(val == 0)) { : *result = PARSE_UNDERFLOW; : } else { : *result = PARSE_FAILURE; : return 0; : } : return (T)(negative ? -val : val); : } : : // check if trailing characters are present and are all white spaces : int trailing_len = processed_str.length() - (int)(s_end - c_str); : i > Maybe we could put this into its own function, stg like 'NormalizeFloatStri Done...added a new private function and test for it. http://gerrit.cloudera.org:8080/#/c/17389/5/be/src/util/string-parser.h@524 PS5, Line 524: esult = PARSE_OVERFLOW; > Should we also test std::isnan(val) and set PARSE_UNDERFLOW flag here? Actually, when function underflows it doesn't return NaN. It returns 0 instead. Handled that case. -- To view, visit http://gerrit.cloudera.org:8080/17389 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic105ad38a2fcbf2fb4e8ae8af6d9a8e251a9c141 Gerrit-Change-Number: 17389 Gerrit-PatchSet: 6 Gerrit-Owner: Amogh Margoor <[email protected]> Gerrit-Reviewer: Amogh Margoor <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 20 May 2021 22:30:39 +0000 Gerrit-HasComments: Yes
