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

Hello, thanks for the new patch.

0004 failed to be applied on the underneath patches.

At Sun, 5 Nov 2017 15:54:19 +0100, Emre Hasegeli <emre@hasegeli.com> wrote in
<CAE2gYzzngpYgrQbJ-2TjzZ+MZBa0D0Xzj8tjjJLv6C3CPARMGw@mail.gmail.com>
> > I am not sure how useful NaNs are in geometric types context, but we
> > allow them, so inconsistent hypot() would be a problem.  I will change
> > my patches to keep pg_hypot().
> 
> New versions of the patches are attached with 2 additional ones.  The
> new versions leave pg_hypot() in place.  One of the new patches
> improves the test coverage.  The line coverage of geo_ops.c increases
> from 55% to 81%.  The other one fixes -0 values to 0 on float
> operators.  I am not sure about performance implication of this, so
> kept it separate.  It may be a better idea to check this only on the
> platforms that has tendency to produce -0.
> 
> While working on the tests, I found some unreachable code and removed
> it.  I also found that lseg ## lseg operator returning wrong results.
> It is defined as "closest point to first segment on the second
> segment", but:
> 
> > # select '[(1,2),(3,4)]'::lseg ## '[(0,0),(6,6)]'::lseg;
> >  ?column?
> > ----------
> >  (1,2)
> > (1 row)
> 
> I appended the fix to the patches.  This is also effecting lseg ## box operator.

Mmm.. It returns (1.5, 1.5) with the 0004 patch. It is surely a
point on the second operand but I'm not sure it's right that the
operator returns a specific point for two parallel segments.

> I also changed recently band-aided point ## lseg operator to return
> the point instead of NULL when it cannot find the correct result to
> avoid the operators depending on this one to crash.

I'd like to put comments on 0001 and 0004 only now:
- Adding [LR]DELIM_L looks good but they should be described in  the comment just above.
- Renaming float8_slope to slope seems no problem.
- I'm not sure the change of box_construct is good but currently  all callers fits the new interface (giving two
points,not  four coordinates).
 
- close_lseg seems forgetting the case where the two given  segments are crossing (and parallel). For example,
  select '(-8,-8),(1,1)'::lseg ## '(-10,0),(2,0)'::lseg;
  is expected to return (0,0), which is the crossing point of  the two segments, but it returns (1,0) (and returned
(1,1) before the patch), which is the point on the l2 closest to the  closer end of l1 to l2.
 
  As mentioned above, it is difficult to dicide what is the  proper result for parallel segments. I suppose that the
followingoperation should return an invalid value as a point.
 
  select '(-1,0),(1,0)'::lseg ## '(-1,1),(1,1)'::lseg;
  close_lseg does the following operations in the else case of  'if (float8_...)'. If i'm not missing something, the
resultof  the following operation is always the closer end of l2. In  other words it seems a waste of cycles.    |
point= DirectFunctionCall2(close_ps,  |                             PointPGetDatum(&l2->p[closer_end2]),  |
               LsegPGetDatum(l1));  | return DirectFunctionCall2(close_ps, point, LsegPGetDatum(l2));
 


- make_bound_box operates directly on the poly->boundbox. I'm afraid that such coding hinders compiers from using
registers.
 This is a bit apart from this patch, it would be better if we could eliminate internal calls using
DirectFunctionCall.

reagrds,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers