Thread: NaN support in NUMERIC data type
Hi, I've just noticed that the NUMERIC input function special cases NaN values differently to the floating point input functions. For example the following are all accepted (on my system anyway): SELECT 'NaN'::float8; SELECT ' NaN'::float8; SELECT 'NaN '::float8; SELECT '+NaN'::float8; SELECT '-NaN'::float8; whereas only the first is OK for numeric. Is this deliberate? A quick check of utils/adt/numeric.c would suggest that it's been special cased as a optimisation so we don't allocate a numeric value in set_var_from_str() unless we need to. As a side note; I'm only really interested in the leading/trailing spaces. I only noticed the leading plus/minus sign when reading the code and think it's probably better if a NaN is rejected if it has a leading sign. I think it would be better if it was consistent with FLOAT, not sure how to do this in a platform independent way though. I could submit a patch if you want; I'm unsure whether it's better to duplicate code in numeric_in, do slightly more work and allocate memory for NaN's when not strictly needed, or remove knowledge of NumericVar from numeric_in altogether and push code into set_var_from_str. Comments? -- Sam http://samason.me.uk/
Sam Mason <sam@samason.me.uk> writes:
> I've just noticed that the NUMERIC input function special cases NaN
> values differently to the floating point input functions. For example
> the following are all accepted (on my system anyway):
> SELECT 'NaN'::float8;
> SELECT ' NaN'::float8;
> SELECT 'NaN '::float8;
> SELECT '+NaN'::float8;
> SELECT '-NaN'::float8;
> whereas only the first is OK for numeric. Is this deliberate?
Well, the +- part must be an artifact of your strtod() implementation;
our own code isn't doing anything to accept that. I think it's pretty
bogus --- NaNs do not have signs.
IIRC, the explicit support for leading/trailing spaces is something that
we added in float8in long after numeric_in was written, and I think just
nobody thought about numeric at the time. But it's clearly inconsistent
to allow spaces around a regular value but not a NaN.
Possibly the logic for leading/trailing spaces could be pulled out
of set_var_from_str and executed in numeric_in instead?
regards, tom lane
On Tue, Apr 07, 2009 at 12:51:21PM -0400, Tom Lane wrote: > Sam Mason <sam@samason.me.uk> writes: > > SELECT 'NaN'::float8; > > SELECT ' NaN'::float8; > > SELECT 'NaN '::float8; > > SELECT '+NaN'::float8; > > SELECT '-NaN'::float8; > > Well, the +- part must be an artifact of your strtod() implementation; > our own code isn't doing anything to accept that. I think it's pretty > bogus --- NaNs do not have signs. OK, didn't actually check the code with that; no point worrying about it then. > IIRC, the explicit support for leading/trailing spaces is something that > we added in float8in long after numeric_in was written, and I think just > nobody thought about numeric at the time. But it's clearly inconsistent > to allow spaces around a regular value but not a NaN. Good, I wanted to make sure it wasn't a deliberate thing before doing too much. > Possibly the logic for leading/trailing spaces could be pulled out > of set_var_from_str and executed in numeric_in instead? Yes, I didn't want to do this before because it's called from a couple of other places. I looked again and realised that we're generating those in very fixed formats so don't need to worry about leading/trailing spaces and hence can move the code up to numeric_in. The attached patch gives set_var_from_str an "endptr" similar to strtod so handling is closer to the float[48]in code. I moved error reporting code outside as well to cut down on the multiple identical calls. The included patch was generated against 8.3.5 (because that's what I had lying around when I started playing) but applies with a little fuzz to the latest snapshot and does the right thing for me in both versions. -- Sam http://samason.me.uk/
Attachment
Sam Mason <sam@samason.me.uk> writes:
> On Tue, Apr 07, 2009 at 12:51:21PM -0400, Tom Lane wrote:
>> IIRC, the explicit support for leading/trailing spaces is something that
>> we added in float8in long after numeric_in was written, and I think just
>> nobody thought about numeric at the time. But it's clearly inconsistent
>> to allow spaces around a regular value but not a NaN.
> The included patch was generated against 8.3.5 (because that's what I
> had lying around when I started playing) but applies with a little fuzz
> to the latest snapshot and does the right thing for me in both versions.
Hmm, did it do the right thing for NaN with a typmod? I doubt
apply_typmod is safe for a NaN. Anyway, I revised this a bit and
applied to HEAD. I'm disinclined to back-patch; it's not really a bug
but a definition change, and we get flack when we put that sort of
change into stable branches ...
regards, tom lane
On Wed, Apr 08, 2009 at 06:11:59PM -0400, Tom Lane wrote: > Sam Mason <sam@samason.me.uk> writes: > > On Tue, Apr 07, 2009 at 12:51:21PM -0400, Tom Lane wrote: > >> IIRC, the explicit support for leading/trailing spaces is something that > >> we added in float8in long after numeric_in was written, and I think just > >> nobody thought about numeric at the time. But it's clearly inconsistent > >> to allow spaces around a regular value but not a NaN. > > > The included patch was generated against 8.3.5 (because that's what I > > had lying around when I started playing) but applies with a little fuzz > > to the latest snapshot and does the right thing for me in both versions. > > Hmm, did it do the right thing for NaN with a typmod? I doubt > apply_typmod is safe for a NaN. Oops, good catch. Didn't think to check for that. > Anyway, I revised this a bit and applied to HEAD. I've not tested; but your changes look as though they will break: SELECT 'Infinity'::float::numeric; I think you'll get '0' back instead of an error; either that or it's too late for me to be thinking straight! > I'm disinclined to back-patch; it's not really a bug > but a definition change, and we get flack when we put that sort of > change into stable branches ... OK Out of personal interest; why did you translate: while(isspace(*p)) p++; into more verbose forms? Is it so it fits in with the style in rest of the code, or does it actually do something different that I'm missing? -- Sam http://samason.me.uk/
Sam Mason <sam@samason.me.uk> writes:
> On Wed, Apr 08, 2009 at 06:11:59PM -0400, Tom Lane wrote:
>> Anyway, I revised this a bit and applied to HEAD.
> I've not tested; but your changes look as though they will break:
> SELECT 'Infinity'::float::numeric;
That gives an error now, just as it did before, so I'm not sure what you
think is broken about it ...
> Out of personal interest; why did you translate:
> while(isspace(*p)) p++;
> into more verbose forms?
I just copied-and-pasted what was there before (inside the subroutine).
Didn't see much reason to change it.
regards, tom lane
On Wed, Apr 08, 2009 at 08:16:52PM -0400, Tom Lane wrote: > Sam Mason <sam@samason.me.uk> writes: > > On Wed, Apr 08, 2009 at 06:11:59PM -0400, Tom Lane wrote: > >> Anyway, I revised this a bit and applied to HEAD. > > > I've not tested; but your changes look as though they will break: > > SELECT 'Infinity'::float::numeric; > > That gives an error now, just as it did before, so I'm not sure what you > think is broken about it ... I shouldn't have responded last night; didn't realise how little of my code was left so the semantics I was assuming weren't valid. -- Sam http://samason.me.uk/