Re: [HACKERS] [PATCH] Improve geometric types

Hello,

I played more around geometric types and recalled that geometric
types don't have orderings. Also have corrected some other
misunderstanding. Sorry for my confusion and I think I returned
on the way.

At Wed, 17 Jan 2018 18:59:30 +0100, Emre Hasegeli <emre(at)hasegeli(dot)com> wrote in <CAE2gYzzgJ7B-HdmxuSDoqu-_x1nEeoEA45is2hP8ex4r3KNH8Q(at)mail(dot)gmail(dot)com>
> > I'm not sure what you mean by the "basic comparison ops" but I'm
> > fine with the policy, handling each component values in the same
> > way with float. So point_eq_point() is right and other functions
> > should follow the same policy.
>
> I mean <, >, <= and >= by basic comparison operators. Operators with
> those symbols are available for some geometric types, but they are
> comparing the sizes of the objects. Currently only the equality
> operators follow the same policy with point_eq_point (), others never
> return true when NaNs are involved.
>
> > Sorry for going back and force, but I don't see a difference
> > between it and the original behavior from the point of view of
> > reasonability. Isn't is enough to let each component comparison
> > follow float by redefining FPxx() functions?
>
> My previous patch was doing exactly that. I though that is not what
> we want to do. Do we want box_overlap() to return true when NaNs are
> involved?

All comparison operators don't need consideration on
ordering. And the existing comparison operators behaves
diferrently from float and already behaves in the way of bare
float. There's no behavior as the whole of an object. I have
fixed my understanding as this. (And I saw all patches altoghter
by mistake..) As the result most my concern was pointless.

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

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

0002: I have no comment so far on this patch.

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.

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

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?

Assertion is added in pg_hypot but it is impossible for result
to be negative there. Why do you added 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..

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.

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

-- Sorry time's up today.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center