Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> I had a look at this, and I think it's mostly in good shape. It looks
> like everything from the first message in this thread has been
> resolved, except I don't know about the jsonpath stuff, because I
> haven't been following that.
Thanks for the careful review! Yeah, Alexander fixed the jsonpath
stuff at df646509f, so I think all my original concerns are cleared,
other than the question of whether to invent isfinite() and isnan()
SQL functions. That seems like follow-on work in any case.
> 1). I don't think that the way in_range() handles infinities is quite
> right. For example:
> SELECT in_range('inf'::numeric, 10::numeric, 'inf'::numeric, false, false);
> in_range
> ----------
> f
> (1 row)
> But I think that should return "val >= base + offset", which is "Inf
> >= Inf", which should be true.
Hmm. I modeled the logic on the float8 in_range code, which does the
same thing:
# SELECT in_range('inf'::float8, 10::float8, 'inf'::float8, false, false);
in_range
----------
f
(1 row)
It does seem like this is wrong per the specification of in_range, though,
so do we have a bug to fix in the float in_range support? If so I'd
be inclined to go correct that first and then adapt the numeric patch
to match.
> Similarly, I think this should return true:
> SELECT in_range('-inf'::numeric, 10::numeric, 'inf'::numeric, true, true);
Same comment.
> I think this could use some test coverage.
Evidently :-(
> 2). I think numeric_pg_lsn() needs updating -- this should probably be an error:
Oh, that was not there when I produced my patch. Will cover it in the
next version.
I agree with your other comments and will update the patch.
> Finally, not really in the scope of this patch, but something I
> noticed anyway while looking at edge cases -- float and numeric handle
> NaN/0 differently:
> SELECT 'nan'::float8 / 0::float8;
> ERROR: division by zero
> SELECT 'nan'::numeric / 0::numeric;
> ?column?
> ----------
> NaN
Hmm. It seems like we generally ought to try to follow IEEE 754
for the semantics of operations on NaN, but I don't have a copy of
that spec so I'm not sure which result it specifies for this.
I agree that being inconsistent between the two types is not what
we want.
regards, tom lane
