Attila Jeges has posted comments on this change. Change subject: IMPALA-1731,IMPALA-3868: Float values are not parsed correctly ......................................................................
Patch Set 4: (4 comments) There is a benchmark program (atof-benchmark) to test the performance of StringToFloat. The performance degraded a bit. Before applying the change the number of iterations per millisecond was about 11.2, after the change it is 8.33. StringToFloat still performs better than atof though. atof does 5.19 iterations per millisecond. http://gerrit.cloudera.org:8080/#/c/3622/4//COMMIT_MSG Commit Message: PS4, Line 7: MPALA > IMPALA Done PS4, Line 12: Trailing > leading too Actually, StringParser class has two static functions that deal with float parsing: StringToFloat and StringToFloatInternal. StringToFloat is public, StringToFloatInternal is private. StringToFloatInternal is merely a helper function to StringToFloat. StringToFloat skips leading whitespace and then simply calls StringToFloatInternal. Thus, StringToFloat accepts leading whitespace while StringToFloatInternal does not. The commit message talks only about StringToFloatInternal. Since StringToFloatInternal is private, we cannot test it directly. We can only test StringToFloat, but then we have to test that it works with leading whitespace as well. PS4, Line 12: strings like > remove "strings like". These are the only strings. Done PS4, Line 16: Trailing > leading too Same as above. -- To view, visit http://gerrit.cloudera.org:8080/3622 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9e17d0f051b300a22a520ce34e276c2d4460d35e Gerrit-PatchSet: 4 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Attila Jeges <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-HasComments: Yes
