Attila Jeges has posted comments on this change. Change subject: IMPALA-1731: StringToFloatInternal() does not handle inf correctly ......................................................................
Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/3622/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 2788: numeric_limits<double>::infinity()); > I'm curious what happens if you do this the other way round (1/0 -> double It returns numeric_limits<float>::infinity(). I've added a test to verify this works. Line 2797: // Parsing inf values is case-insensitive. > Can you add tests for broken strings like null character in the middle, emp Done. Line 2798: TestValue("CAST('iNf' AS FLOAT)", TYPE_FLOAT, > I don't think we need 3 upper/lower case permutations, 1 should be sufficie Before this patch-set, parsing inf and NaN values in a case-insensitive manner was done by comparing characters one-by-one (instead of just calling strncasecmp), so Michael asked me to test all these different variations. http://gerrit.cloudera.org:8080/#/c/3622/1/be/src/util/string-parser.h File be/src/util/string-parser.h: Line 372: case '-': negative = true; > Add a comment to explain that the fallthrough is intended. Done PS1, Line 377: Hive writes inf as Infinity, at least for text. We'll be a little loose : // here and interpret any column with "inf" as a prefix as infinity rather than : // checking every remaining byte. > Do you think it's worth restricting this to "inf" and "infinity" only in ne Yes, we should definitely do that PS1, Line 422: } else if (s[i] == '.') { : decimal = true; > I meant cast("2.3....3" as float) is treated the same as cast("2.33" as flo I've created a new JIRA ticket (IMPALA-3868) and fixed it in the next patch-set. -- 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: 1 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: Michael Ho <[email protected]> Gerrit-HasComments: Yes
