Horton

Re: [HACKERS] [PATCH] Improve geometric types

Emre Hasegeli <emre@hasegeli.com>

View thread View on postgr.es
From: Emre Hasegeli <emre@hasegeli.com>
To: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Álvaro Herrera <alvherre@alvh.no-ip.org>, Robert Haas <robertmhaas@gmail.com>, Aleksander Alekseev <a.alekseev@postgrespro.ru>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2018-02-01T09:35:42Z
Lists: pgsql-hackers 
> 1."COPT=-DGEODEBUG make" complains as follows.
>
>   | geo_ops.c:2445:62: error: invalid type argument of unary ‘*’ (have ‘float8 {aka double}’)
>   |   printf("dist_ppoly_internal- segment 0/n distance is %f\n", *result);

Fixing.

> 2. line_construct_pm has been renamed to line_construct. I
>    noticed that the patch adds the second block for "(m == 0.0)"
>    (from the ealier versions) but it seems to work exactly as the
>    same to the "else" block. We need a comment about the reason
>    for the seemingly redundant second block.

It is indeed redundant.  I am removing it.

> 3. point_sl can return -0.0 and that is a thing that this patch
>    intends to avoid. line_invsl has the same problem.

The existing version of the function has the same issue.  I think we
need a global solution for -0s.  See the float-zero patch.

> 4. lseg_interpt_line is doing as follows.
>
>    >  if (FPeq(lseg->p[0].x, interpt.x) && FPeq(lseg->p[0].y, interpt.y))
>    >    *result = lseg->p[0];
>    >  else if (FPeq(lseg->p[1].x, interpt.x) && FPeq(lseg->p[1].y, interpt.y))
>    >    *result = lseg->p[1];
>
>    I suppose we can use point_pt_point for this purpose.

Yes, I am changing it.

>    >  if (point_eq_point(&lseg->p[0], &interpt))
>    >    *result = lseg->p[0];
>    >  else if (point_eq_point(&lseg->p[1], &interpt))
>    >    *result = lseg->p[1];
>
>    However I'm not sure that adjusting the intersection to the
>    tips of the segment is good or not. Adjusting onto the line
>    can be better in another case. lseg_interpt_lseg, for
>    instance, checks lseg_contain_point on the line parameter of
>    lseg_interpt_line.

Me neither, but it is probably better than returning a point that
extends the endpoints of the segment.  I am inclined to leave it alone
for now.

Commits

  1. Improve test coverage of geometric types

  2. Fix problems in handling the line data type

  3. Use the built-in float datatypes to implement geometric types

  4. Remove remaining GEODEBUG references from geo_ops.c

  5. Provide separate header file for built-in float types

  6. Refactor geometric functions and operators

  7. Fix crash in close_ps() for NaN input coordinates.

  8. Fix GiST index build for NaN values in geometric types.

  9. Enable building with Visual Studion 2013.

  10. Suppress -0 in the C field of lines computed by line_construct_pts().