impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Attila Jeges (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) IMPALA-1731,IMPALA-3868: Float values are not parsed correctly
Date Sat, 30 Jul 2016 19:35:57 GMT
Attila Jeges has posted comments on this change.

Change subject: IMPALA-1731,IMPALA-3868: Float values are not parsed correctly

Patch Set 4:


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.
Commit Message:

PS4, Line 7: MPALA

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.

PS4, Line 16: Trailing
> leading too
Same as above.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e17d0f051b300a22a520ce34e276c2d4460d35e
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Attila Jeges <>
Gerrit-Reviewer: Attila Jeges <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Lars Volker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-HasComments: Yes

View raw message