Re: [HACKERS] [PATCH] Improve geometric types - Mailing list pgsql-hackers

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