Thread: Fix typo function circle_same (src/backend/utils/adt/geo_ops.c)

> On 2 Sep 2022, at 21:08, Ranier Vilela <ranier.vf@gmail.com> wrote:

> At function circle_same the second isnan test is wrong.

Yeah, that seems pretty wrong.  Did you attempt to procure a test for when this
yields the wrong result?

--
Daniel Gustafsson        https://vmware.com/

> On 2 Sep 2022, at 21:22, Ranier Vilela <ranier.vf@gmail.com> wrote:
>
> Em sex., 2 de set. de 2022 às 16:15, Daniel Gustafsson <daniel@yesql.se <mailto:daniel@yesql.se>> escreveu:
> > On 2 Sep 2022, at 21:08, Ranier Vilela <ranier.vf@gmail.com <mailto:ranier.vf@gmail.com>> wrote:
>
> > At function circle_same the second isnan test is wrong.
>
> Yeah, that seems pretty wrong.  Did you attempt to procure a test for when this
> yields the wrong result?
> Hi Daniel,
> Unfortunately not.

On HEAD, the below query yields what seems to be the wrong result for the "same
as" operator:

postgres=# select '<(0,0),NaN>'::circle ~= '<(0,0),1>'::circle;
 ?column?
----------
 t
(1 row)

With the patch applied, it returns the expected:

postgres=# select '<(0,0),NaN>'::circle ~= '<(0,0),1>'::circle;
 ?column?
----------
 f
(1 row)

There seems to be surprisingly few tests around these geo operators?

This was introduced in c4c340088 so any fix needs to be backpatched to v12.  I
will do some more testing and digging to verify and will take care of it.

--
Daniel Gustafsson        https://vmware.com/

Hi,

On Fri, Sep 01, 2022 at 09:55:15PM +0200, Daniel Gustafsson wrote:
> > On 2 Sep 2022, at 21:22, Ranier Vilela <ranier.vf@gmail.com> wrote:
> >
> > Em sex., 2 de set. de 2022 às 16:15, Daniel Gustafsson <daniel@yesql.se <mailto:daniel@yesql.se>> escreveu:
> > > On 2 Sep 2022, at 21:08, Ranier Vilela <ranier.vf@gmail.com <mailto:ranier.vf@gmail.com>> wrote:
> >
> > > At function circle_same the second isnan test is wrong.
> >
> > Yeah, that seems pretty wrong.  Did you attempt to procure a test for when this
> > yields the wrong result?
> > Hi Daniel,
> > Unfortunately not.
>
> On HEAD, the below query yields what seems to be the wrong result for the "same
> as" operator:
>
> postgres=# select '<(0,0),NaN>'::circle ~= '<(0,0),1>'::circle;
>  ?column?
> ----------
>  t
> (1 row)
>
> With the patch applied, it returns the expected:
>
> postgres=# select '<(0,0),NaN>'::circle ~= '<(0,0),1>'::circle;
>  ?column?
> ----------
>  f
> (1 row)
>
> There seems to be surprisingly few tests around these geo operators?

Yeah, there are unfortunately a lot of problems around those and NaN, with
multiple reports in the past (I recall [1] and [2] but there were others).
There was a CF entry that tried to improve things [3], part of it was committed
but not all [4], and clearly some more work is needed.

[1] https://www.postgresql.org/message-id/flat/CAGf+fX70rWFOk5cd00uMfa__0yP+vtQg5ck7c2Onb-Yczp0URA@mail.gmail.com
[2] https://www.postgresql.org/message-id/20210330095751.x5hnqbqcxilzwjlm@nol
[3] https://commitfest.postgresql.org/38/2710/
[4] https://www.postgresql.org/message-id/3558828.1659045056@sss.pgh.pa.us

> On 3 Sep 2022, at 09:36, Julien Rouhaud <rjuju123@gmail.com> wrote:

> Yeah, there are unfortunately a lot of problems around those and NaN, with
> multiple reports in the past (I recall [1] and [2] but there were others).

NaNs are indeed incredibly complicated, but I think we are sort of in a good
place here given it's testing for equality in floats.  The commit message of
c4c34008854654279ec30067d72fc5d174d2f42f carries an explanation:

    The float datatypes consider NaNs values to be equal and greater than
    all non-NaN values.  This change considers NaNs equal only for equality
    operators.  The placement operators, contains, overlaps, left/right of
    etc.  continue to return false when NaNs are involved.

From testing and reading I believe the fix in this thread is correct, but since
NaNs are involved I will take another look at this with fresh eyes before going
ahead.

--
Daniel Gustafsson        https://vmware.com/

> On 4 Sep 2022, at 14:39, Ranier Vilela <ranier.vf@gmail.com> wrote:

> But with Windows 10 build, I got this diff result:
>
> diff -w -U3 C:/dll/postgres_dev/postgres_master/src/test/regress/expected/geometry.out
C:/dll/postgres_dev/postgres_master/src/test/regress/results/geometry.out
> --- C:/dll/postgres_dev/postgres_master/src/test/regress/expected/geometry.out 2022-09-01 08:05:03.685931000 -0300
> +++ C:/dll/postgres_dev/postgres_master/src/test/regress/results/geometry.out 2022-09-04 09:27:47.133617800 -0300
> @@ -4380,9 +4380,8 @@
>   <(100,200),10> | <(100,200),10>
>   <(100,1),115>  | <(100,1),115>
>   <(3,5),0>      | <(3,5),0>
> - <(3,5),NaN>    | <(3,5),0>
>   <(3,5),NaN>    | <(3,5),NaN>
> -(9 rows)
> +(8 rows)
>
>  -- Overlap with circle
>  SELECT c1.f1, c2.f1 FROM CIRCLE_TBL c1, CIRCLE_TBL c2 WHERE c1.f1 && c2.f1;
>
> Not sure why.

That's not just on Windows, and it makes total sense as a radius of NaN isn't
the same as a radius of zero.

--
Daniel Gustafsson        https://vmware.com/