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

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@hasegeli.com> wrote in
<CAE2gYzzgJ7B-HdmxuSDoqu-_x1nEeoEA45is2hP8ex4r3KNH8Q@mail.gmail.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