impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Attila Jeges (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-1731: StringToFloatInternal() does not handle inf correctly
Date Mon, 18 Jul 2016 16:02:10 GMT
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 <attilaj@cloudera.com>
Gerrit-Reviewer: Attila Jeges <attilaj@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message