Attila Jeges has posted comments on this change. Change subject: IMPALA-1731,IMPALA-3868: Float values are not parsed correctly ......................................................................
Patch Set 3: (5 comments) I will upload the new patch-set to Impala-ASF project, master branch http://gerrit.cloudera.org:8080/#/c/3622/3//COMMIT_MSG Commit Message: PS3, Line 12: strings > with trailing whitespace, right? Yes, I've changed the message to make that clear. http://gerrit.cloudera.org:8080/#/c/3622/3/be/src/util/string-parser.h File be/src/util/string-parser.h: PS3, Line 438: else if (!decimal && s[i] == '.') { : decimal = true; : } > I think this may allow multiple dots if the following character is e or E, It handles the "0..e" case. When it encounters the second dot, the else block gets executed (line 442), IsAllWhitespace(s + i, len - i)) returns false, so we return with PARSE_FAILURE. I've added a new BE test to confirm that it works. PS3, Line 443: first > probably easier to read if this is just 1 and 'first' is removed first is either 1 or 0 depending on whether 's' started with a +/- sign or not. http://gerrit.cloudera.org:8080/#/c/3622/3/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: PS3, Line 2459: cast('inf' as double), cast('InFinity' as float), > please also add positive cases for 'inf ' and 'infinity ' w/ trailing white Done Actually, leading white-space is allowed here too. Leading white-space is considered to be invalid only in StringToFloatInternal(). Line 2467: # IMPALA-1731: test parsing NaN values > same about trailing whitespace Done -- 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: 3 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
