Lars Volker has posted comments on this change. Change subject: IMPALA-1731: StringToFloatInternal() does not handle inf correctly ......................................................................
Patch Set 1: (4 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 -> float). Line 2797: // Parsing inf values is case-insensitive. Can you add tests for broken strings like null character in the middle, empty strings +inf and +nan? Please check if they're covered somewhere else in the file before doing so. Line 2798: TestValue("CAST('iNf' AS FLOAT)", TYPE_FLOAT, I don't think we need 3 upper/lower case permutations, 1 should be sufficient. Is there a reason why each one is needed? 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. -- 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
