Re: [PATCH] Improve geometric types

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(at)hasegeli(dot)com> wrote in <CAE2gYzzngpYgrQbJ-2TjzZ+MZBa0D0Xzj8tjjJLv6C3CPARMGw(at)mail(dot)gmail(dot)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
following operation 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 result of
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