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 <e...@hasegeli.com> wrote in <cae2gyzzgj7b-hdmxusdoqu-_x1neeoea45is2hp8ex4r3kn...@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