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: 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:

File be/src/exprs/

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

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.
File be/src/util/string-parser.h:

Line 372:       case '-': negative = true;
> Add a comment to explain that the fallthrough is intended.

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
             :     // 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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e17d0f051b300a22a520ce34e276c2d4460d35e
Gerrit-PatchSet: 1
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: Michael Ho <>
Gerrit-HasComments: Yes

View raw message