Re: [PATCH] Improve geometric types

Hello,

> I'd like to put comments on 0001 and 0004 only now:
...

I don't have a comment on 0002.

About 0003:

> @@ -4487,21 +4486,21 @@ circle_in(PG_FUNCTION_ARGS)
> ...
> circle->radius = single_decode(s, &s, "circle", str);
> - if (circle->radius < 0)
> + if (float8_lt(circle->radius, 0.0))
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),

flost8_lt and its family functions are provided to unify the
sorting order including NaN. NaN is not rejected by the usage of
float8_lt in the case but it is what the function is expected to
be used for. If we wanted to check if it is positive, it
unexpectedly throws an exception. (I suppose that NaNs should be
silently ignored rather than stopping a query by throwng an
exception.)

Addition to that I don't think it proper that applying EPSILON(!)
there. It should be strictly compared regardless whether EPSION
is considered or not.

Similary, circle_overlap for example, float8_le is used to
compare the distance and the summed radius.

NaN causes a problem in another place.

> PG_RETURN_BOOL(FPle(point_dt(&circle1->center, &circle2->center),
> float8_pl(circle1->radius, circle2->radius)));

If the distance was NaN and the summed radius is not, the
function returns true. I think that a reasonable behavior is that
an object containing NaN cannot make any meaningful relationship
with other objects as floating number itself behave so. (Any
comparison other than != with NaN returns always false)

Just using another series of comparison functions that return
false for NaN-containing comparison is not a solution since the
meaning of the returned false differs by context, just same as
the first problem above. For exameple, the fictious functions
below,

| bool circle_overlaps()
| ret = FPle(distance, radius_sum);

This gives correct results, but

| bool circle_not_overlaps()
| ret = FPgt(distance, radius_sum);

This gives a wrong result for NaN-containing objects.

Perhaps it is enough to explicitly define behaviors for NaN
before comparison.

circle_overlap()
> distance = point_dt(....);
> radius_sum = float8_pl(...);
>
> /* NaN-containing objects doesn't overlap any other objects */
> if (isnan(distance) || isnan(radius_sum))
> PG_RETURN_BOOL(false);
>
> /* NaN ordering of FPle() doesn't get into mischief here */
> return PG_RETURN_BOOL(FPle(distance, radius_sum));

(End Of the Comment to 0003)

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center