From 677ae0293d35e4f64f359a3bc1ace9208540e5fb Mon Sep 17 00:00:00 2001 From: Oliver Tan Date: Fri, 14 Jul 2023 08:57:24 +1000 Subject: [PATCH] geoindex: error indexing NaN coordinates Release note (bug fix): Previously, inserting geometries into a table with an inverted index involving a NaN coordinate could result in a panic. This has now errors instead. --- .../tests/3node-tenant/generated_test.go | 7 +++++++ pkg/geo/geoindex/BUILD.bazel | 2 ++ pkg/geo/geoindex/s2_geometry_index.go | 10 ++++++++++ pkg/geo/geoindex/testdata/s2_geometry | 17 +++++++++++++++++ .../testdata/logic_test/geospatial_regression | 9 +++++++++ .../tests/fakedist-disk/generated_test.go | 7 +++++++ .../tests/fakedist-vec-off/generated_test.go | 7 +++++++ .../logictest/tests/fakedist/generated_test.go | 7 +++++++ .../generated_test.go | 7 +++++++ .../local-mixed-22.2-23.1/generated_test.go | 7 +++++++ .../tests/local-vec-off/generated_test.go | 7 +++++++ pkg/sql/logictest/tests/local/generated_test.go | 7 +++++++ 12 files changed, 94 insertions(+) create mode 100644 pkg/sql/logictest/testdata/logic_test/geospatial_regression diff --git a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go index c059ea9dafee..43c3cc8c5d1a 100644 --- a/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go +++ b/pkg/ccl/logictestccl/tests/3node-tenant/generated_test.go @@ -871,6 +871,13 @@ func TestTenantLogic_geospatial_meta( runLogicTest(t, "geospatial_meta") } +func TestTenantLogic_geospatial_regression( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "geospatial_regression") +} + func TestTenantLogic_geospatial_zm( t *testing.T, ) { diff --git a/pkg/geo/geoindex/BUILD.bazel b/pkg/geo/geoindex/BUILD.bazel index 14bce47bf8e1..978d9ae53e9f 100644 --- a/pkg/geo/geoindex/BUILD.bazel +++ b/pkg/geo/geoindex/BUILD.bazel @@ -21,6 +21,8 @@ go_library( "//pkg/geo/geopb", "//pkg/geo/geoprojbase", "//pkg/geo/geos", + "//pkg/sql/pgwire/pgcode", + "//pkg/sql/pgwire/pgerror", "@com_github_cockroachdb_errors//:errors", "@com_github_golang_geo//r3", "@com_github_golang_geo//s1", diff --git a/pkg/geo/geoindex/s2_geometry_index.go b/pkg/geo/geoindex/s2_geometry_index.go index fb603a5c656c..397b3a0ec127 100644 --- a/pkg/geo/geoindex/s2_geometry_index.go +++ b/pkg/geo/geoindex/s2_geometry_index.go @@ -19,6 +19,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/geo/geopb" "github.com/cockroachdb/cockroach/pkg/geo/geoprojbase" "github.com/cockroachdb/cockroach/pkg/geo/geos" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/errors" "github.com/golang/geo/r3" "github.com/golang/geo/s2" @@ -277,6 +279,14 @@ func (s *s2GeometryIndex) DFullyWithin( // Converts to geom.T and clips to the rectangle bounds of the index. func (s *s2GeometryIndex) convertToGeomTAndTryClip(g geo.Geometry) (geom.T, bool, error) { + // Anything with a NaN coordinate should be marked as clipped. + if bbox := g.BoundingBoxRef(); bbox != nil && (math.IsNaN(bbox.LoX) || math.IsNaN(bbox.HiX) || + math.IsNaN(bbox.LoY) || math.IsNaN(bbox.HiY)) { + return nil, false, pgerror.Newf( + pgcode.InvalidParameterValue, + "cannot index a geometry with NaN coordinates", + ) + } gt, err := g.AsGeomT() if err != nil { return nil, false, err diff --git a/pkg/geo/geoindex/testdata/s2_geometry b/pkg/geo/geoindex/testdata/s2_geometry index 1043f65cb6f3..efbdc7ae4918 100644 --- a/pkg/geo/geoindex/testdata/s2_geometry +++ b/pkg/geo/geoindex/testdata/s2_geometry @@ -16,6 +16,10 @@ geometry name=poly1 POLYGON((15 15,16 15.5,15.5 17,15 15)) ---- +geometry name=nan +0105000000060000000102000000020000002CF565DB1790F041010000000000F87F1C1E119D614AE7C1010000000000F87F010200000003000000809745B2972CF841010000000000F87FE0D03302F1C6D641010000000000F87FF83C7FD5C4ACD641010000000000F87F010200000004000000C4A695BE1FC2FD41010000000000F87F42F59F55FD780042010000000000F87FA07574B08DC0D941010000000000F87FD843A641FF49E741010000000000F87F010200000004000000781D60294113F241010000000000F87F100451CE196BED41010000000000F87F1430818C69A2EF41010000000000F87F00E4B02D00CB9341010000000000F87F01020000000400000040B0B6B74C96A0C1010000000000F87F1CD943F294E9E5C1010000000000F87F709F7E29F32AC4C1010000000000F87F607737DC4662E941010000000000F87F010200000004000000F097AF975517F441010000000000F87F5442BA391710F841010000000000F87FE01C9C5F7602B641010000000000F87F40E714D68300E2C1010000000000F87F +---- + geometry name=poly1-different-orientation POLYGON((15 15,15.5 17,16 15.5,15 15)) ---- @@ -23,6 +27,19 @@ POLYGON((15 15,15.5 17,16 15.5,15 15)) init minlevel=0 maxlevel=30 maxcells=4 minx=10 miny=10 maxx=20 maxy=20 ---- +# NaN +covers name=nan +---- +cannot index a geometry with NaN coordinates + +index-keys name=nan +---- +cannot index a geometry with NaN coordinates + +intersects name=nan +---- +cannot index a geometry with NaN coordinates + # The orientation difference between the polygons has no effect on the # index keys. diff --git a/pkg/sql/logictest/testdata/logic_test/geospatial_regression b/pkg/sql/logictest/testdata/logic_test/geospatial_regression new file mode 100644 index 000000000000..baf2e3275c1a --- /dev/null +++ b/pkg/sql/logictest/testdata/logic_test/geospatial_regression @@ -0,0 +1,9 @@ +statement ok +CREATE TABLE regression_81609 ( + g GEOMETRY NOT NULL, + INVERTED INDEX (g) +) + +statement error cannot index a geometry with NaN coordinates +INSERT INTO regression_81609 SELECT st_rotatex('0105000000060000000102000000020000002CF565DB1790F041C2509EC57231F2411C1E119D614AE7C166AFF2D39B030142010200000003000000809745B2972CF84160399714F897E241E0D03302F1C6D6410EFC89BE9FE20042F83C7FD5C4ACD6411854CBFDFF42DE41010200000004000000C4A695BE1FC2FD411CE9BCE66C42D8C142F59F55FD7800424C0D1238165FE041A07574B08DC0D94170D034F5A969EB41D843A641FF49E741B6F0864FF80E0242010200000004000000781D60294113F2412C338441624C02C2100451CE196BED41525C0281389BEEC11430818C69A2EF415C09A4FD752D024200E4B02D00CB9341C047BBFE74DDE44101020000000400000040B0B6B74C96A0C162D23CFAACB8E7C11CD943F294E9E5C187C18248646102C2709F7E29F32AC4C160D1A56EC799FAC1607737DC4662E941ADC9125B944FF6C1010200000004000000F097AF975517F441714AF1717A16F0C15442BA391710F841BEBE416B6780F241E01C9C5F7602B6413CC620A0C9C3E14140E714D68300E2C1A0C4038270B4BAC1':::GEOMETRY::GEOMETRY, '-Inf':::FLOAT8)::GEOMETRY; + diff --git a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go index ab65395c96b6..6a59929e8096 100644 --- a/pkg/sql/logictest/tests/fakedist-disk/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-disk/generated_test.go @@ -842,6 +842,13 @@ func TestLogic_geospatial_meta( runLogicTest(t, "geospatial_meta") } +func TestLogic_geospatial_regression( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "geospatial_regression") +} + func TestLogic_geospatial_zm( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go index 3720fab4b21e..cddb1140ea1c 100644 --- a/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist-vec-off/generated_test.go @@ -842,6 +842,13 @@ func TestLogic_geospatial_meta( runLogicTest(t, "geospatial_meta") } +func TestLogic_geospatial_regression( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "geospatial_regression") +} + func TestLogic_geospatial_zm( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/fakedist/generated_test.go b/pkg/sql/logictest/tests/fakedist/generated_test.go index 6d79c2e6e3b9..4020d6b4cbbb 100644 --- a/pkg/sql/logictest/tests/fakedist/generated_test.go +++ b/pkg/sql/logictest/tests/fakedist/generated_test.go @@ -849,6 +849,13 @@ func TestLogic_geospatial_meta( runLogicTest(t, "geospatial_meta") } +func TestLogic_geospatial_regression( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "geospatial_regression") +} + func TestLogic_geospatial_zm( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go b/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go index 034a0e06ecd1..e88c79e2f40d 100644 --- a/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go +++ b/pkg/sql/logictest/tests/local-legacy-schema-changer/generated_test.go @@ -828,6 +828,13 @@ func TestLogic_geospatial_meta( runLogicTest(t, "geospatial_meta") } +func TestLogic_geospatial_regression( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "geospatial_regression") +} + func TestLogic_geospatial_zm( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go b/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go index 8fa551abc51b..8b1253138f8a 100644 --- a/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go +++ b/pkg/sql/logictest/tests/local-mixed-22.2-23.1/generated_test.go @@ -828,6 +828,13 @@ func TestLogic_geospatial_meta( runLogicTest(t, "geospatial_meta") } +func TestLogic_geospatial_regression( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "geospatial_regression") +} + func TestLogic_geospatial_zm( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local-vec-off/generated_test.go b/pkg/sql/logictest/tests/local-vec-off/generated_test.go index 70a53f3fb97d..2cc90bd09aca 100644 --- a/pkg/sql/logictest/tests/local-vec-off/generated_test.go +++ b/pkg/sql/logictest/tests/local-vec-off/generated_test.go @@ -849,6 +849,13 @@ func TestLogic_geospatial_meta( runLogicTest(t, "geospatial_meta") } +func TestLogic_geospatial_regression( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "geospatial_regression") +} + func TestLogic_geospatial_zm( t *testing.T, ) { diff --git a/pkg/sql/logictest/tests/local/generated_test.go b/pkg/sql/logictest/tests/local/generated_test.go index 165c495c46fe..75f8bec21ddb 100644 --- a/pkg/sql/logictest/tests/local/generated_test.go +++ b/pkg/sql/logictest/tests/local/generated_test.go @@ -905,6 +905,13 @@ func TestLogic_geospatial_meta( runLogicTest(t, "geospatial_meta") } +func TestLogic_geospatial_regression( + t *testing.T, +) { + defer leaktest.AfterTest(t)() + runLogicTest(t, "geospatial_regression") +} + func TestLogic_geospatial_zm( t *testing.T, ) {