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 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/17389/7/be/src/util/string-parser-test.cc File be/src/util/string-parser-test.cc: http://gerrit.cloudera.org:8080/#/c/17389/7/be/src/util/string-parser-test.cc@535 PS7, Line 535: TestStringToFloatPreprocess(".43256e4", "0.43256e4"); > May add a couple tests with negative values, such as Preprocess function doesn't handle sign either '+' or '-', so have added it under Invalid input. http://gerrit.cloudera.org:8080/#/c/17389/7/be/src/util/string-parser.h File be/src/util/string-parser.h: http://gerrit.cloudera.org:8080/#/c/17389/7/be/src/util/string-parser.h@508 PS7, Line 508: s[len]) > nit. I wonder if the condition to go without preprocessing (the ElSE branch I didn't understand the conditions above, but if s ='99' (which satisfies condition 2 i believe), it can goto any of the branches based on what s[2] is. if s[2] is null then it will goto true branch and if s[2] is a digit then to false (ELSE) branch. http://gerrit.cloudera.org:8080/#/c/17389/7/be/src/util/string-parser.h@541 PS7, Line 541: return (T)(negative ? -val : val); > nit. This code can not be reached. It will be reached for UNDERFLOW and OVERFLOW. http://gerrit.cloudera.org:8080/#/c/17389/7/be/src/util/string-parser.h@584 PS7, Line 584: > nit. This may not be accurate since the code removes leading '0's before an Agreed. Changed the wordings. -- 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: 8 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: Fri, 09 Jul 2021 23:11:48 +0000 Gerrit-HasComments: Yes
