Qifan Chen 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 7: (4 comments) Looks good to me and thanks a lot for handling well-formed string cases! 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 -0.00001 -.555 -0000.12 -10000000000001 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) is one of the following. 1. '0.' 2. [1-9]+ I was not able to map the above two conditions to the line at 508. 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. http://gerrit.cloudera.org:8080/#/c/17389/7/be/src/util/string-parser.h@584 PS7, Line 584: before '.' nit. This may not be accurate since the code removes leading '0's before any digits. -- 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: 7 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 21:35:17 +0000 Gerrit-HasComments: Yes
