Fix typo function circle_same (src/backend/utils/adt/geo_ops.c)
Hi,
I think that this is a typo.
At function circle_same the second isnan test is wrong.
Attached fix patch.
regards,
Ranier Vilela
Attachments:
0001-fix-typo-isnan-test-geo_ops.patchapplication/octet-stream; name=0001-fix-typo-isnan-test-geo_ops.patchDownload
diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c
index 535301a218..f1b632ef3c 100644
--- a/src/backend/utils/adt/geo_ops.c
+++ b/src/backend/utils/adt/geo_ops.c
@@ -4710,7 +4710,7 @@ circle_same(PG_FUNCTION_ARGS)
CIRCLE *circle1 = PG_GETARG_CIRCLE_P(0);
CIRCLE *circle2 = PG_GETARG_CIRCLE_P(1);
- PG_RETURN_BOOL(((isnan(circle1->radius) && isnan(circle1->radius)) ||
+ PG_RETURN_BOOL(((isnan(circle1->radius) && isnan(circle2->radius)) ||
FPeq(circle1->radius, circle2->radius)) &&
point_eq_point(&circle1->center, &circle2->center));
}
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/
Em sex., 2 de set. de 2022 às 16:15, Daniel Gustafsson <daniel@yesql.se>
escreveu:
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?
Hi Daniel,
Unfortunately not.
regards,
Ranier Vilela
Show quoted text
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]/messages/by-id/CAGf+fX70rWFOk5cd00uMfa__0yP+vtQg5ck7c2Onb-Yczp0URA@mail.gmail.com and [2]/messages/by-id/20210330095751.x5hnqbqcxilzwjlm@nol but there were others).
There was a CF entry that tried to improve things [3]https://commitfest.postgresql.org/38/2710/, part of it was committed
but not all [4]/messages/by-id/3558828.1659045056@sss.pgh.pa.us, and clearly some more work is needed.
[1]: /messages/by-id/CAGf+fX70rWFOk5cd00uMfa__0yP+vtQg5ck7c2Onb-Yczp0URA@mail.gmail.com
[2]: /messages/by-id/20210330095751.x5hnqbqcxilzwjlm@nol
[3]: https://commitfest.postgresql.org/38/2710/
[4]: /messages/by-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/
Em sáb., 3 de set. de 2022 às 19:39, Daniel Gustafsson <daniel@yesql.se>
escreveu:
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.
Yeah, the fix is correct.
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.
regards,
Ranier Vilela
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/
Created a CF entry.
https://commitfest.postgresql.org/40/3883/
Attached a patch with a fix correction to regress output.
I think this needs to be backpatched until version 12.
regards,
Ranier Vilela
Attachments:
v1-0001-fix-typo-isnan-test-geo_ops.patchapplication/octet-stream; name=v1-0001-fix-typo-isnan-test-geo_ops.patchDownload
diff --git a/src/backend/utils/adt/geo_ops.c b/src/backend/utils/adt/geo_ops.c
index 535301a218..f1b632ef3c 100644
--- a/src/backend/utils/adt/geo_ops.c
+++ b/src/backend/utils/adt/geo_ops.c
@@ -4710,7 +4710,7 @@ circle_same(PG_FUNCTION_ARGS)
CIRCLE *circle1 = PG_GETARG_CIRCLE_P(0);
CIRCLE *circle2 = PG_GETARG_CIRCLE_P(1);
- PG_RETURN_BOOL(((isnan(circle1->radius) && isnan(circle1->radius)) ||
+ PG_RETURN_BOOL(((isnan(circle1->radius) && isnan(circle2->radius)) ||
FPeq(circle1->radius, circle2->radius)) &&
point_eq_point(&circle1->center, &circle2->center));
}
diff --git a/src/test/regress/expected/geometry.out b/src/test/regress/expected/geometry.out
index 3b364d1e3b..b50103b216 100644
--- a/src/test/regress/expected/geometry.out
+++ b/src/test/regress/expected/geometry.out
@@ -4380,9 +4380,8 @@ SELECT c1.f1, c2.f1 FROM CIRCLE_TBL c1, CIRCLE_TBL c2 WHERE c1.f1 ~= c2.f1;
<(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;