Re: [HACKERS] [PATCH] Improve geometric types

> 0001:
>
> - You removed the check of parallelity check of
> line_interpt_line(old line_interpt_internal) but line_parallel
> is not changed so the consistency between the two functions are
> lost. It is better to be another patch file (maybe 0004?).

I am making line_parallel() use line_interpt_line(). What they do is
exactly the same.

> - + Assert(p1->npts > 0 && p2->npts > 0);
>
> Other path_ functions are allowing path with no points so just
> returning false if (p1->npts == 0 || p2->npts == 0) seems
> enough. Assertion should be used only for something server
> cannot continue working. (division by zero doesn't crash
> server) I saw other similar assertions in the patch.

path_in() and path_recv() reject paths with no points. I thought we
shouldn't spend cycles for things that cannot happen. I can revert
this part if you find previous practice better.

> 0003:
>
> close_pl/ps/lseg/pb/ls/sb have changed to return null when
> lseg_closept_line returns NAN, but they are defined as strict
> and that is reasonable. Anyway pg_hypot returns NaN only when
> parameters contain INF or NAN so we should error out when it
> returns nan.

I though strict is only related to the input being NULL. Tom Lane
made close_ps() return NULL with commit
278148907a971ec7fa4ffb24248103d8012155d2. The other functions
currently fail with elog()s from DirectFunctionCalls. I don't have
strong preference for NULL or an error. Could you please suggest an
errcode and errmsg, if you have?

> circle_in rejects negative radius and circle_recv modived to
> reject zero and negative. What is the reason for the change?

Overlooking. Thank you for noticing. I am fixing it.

> I understand that we don't need to consider NAN is equal to NAN.
> circle_same, point_eq_point, line_eq no longer needs such
> change?

I though it would be better to consider NaNs equal only on the
equality operators. That is why only they are changed that way.

> Assertion is added in pg_hypot but it is impossible for result
> to be negative there. Why do you added it?

True. I am removing it.

> 0004:
>
> Looking line_recv's change, I became to think that FPxx macros
> is provided for coordinate values. I don't think it is applied
> to non-coordinate values like coefficients. If some kind of
> tolerance is needed here, it is not the same with the EPSILON.
>
> + if (FPzero(line->A) && FPzero(line->B))
> + ereport(ERROR,
>
> So, the above should be exact comparison with 0.0 and line_in
> also should be the same. And maybe the same thing should be done
> at many places..

I agree you. EPSILON is currently applied prematurely. I am trying
to stay away from EPSION related issues to improve the chances to get
this part committed.

> But the following line of line_parallel still requires some kind
> of tolerance to work properly. Since this patch is an
> imoprovement patch, I think we can change it to the vector method.

I am making line_parallel() use line_interpt_line() in response to
your first remark. Vector based algorithm is probably better for
every use of line_interpt_line(), but it is a bigger change with more
user visible effects.

> The problem of line_distance is an existing one so we can leave
> it alone. It returns 0.0 for the most cases but it is a
> long-standing behavior.. (Anyway I don't find a reasonable
> definition of the distance between very-nearly parallel lines..)

Exact comparison with 0.0 instead of FPzero() would also be an
improvement for line_distance(), but I am not doing it now.

> -- Sorry time's up today.

I am waiting for the rest of your review to post the new versions.