From 537f89dc48671ca3a79a1b6d29397b89533a85e9 Mon Sep 17 00:00:00 2001 From: richardjcai Date: Thu, 24 Feb 2022 12:42:28 -0500 Subject: [PATCH] sql, geo: Fix upper case geohash parsing and allow NULL arguments Release note (sql change): ST_Box2DFromGeoHash now accepts NULL arguments, the precision being NULL is the same as no precision being passed in at all. Upper case characters are now parsed as lower case characters for geohash, this matches Postgis behaviour. --- pkg/geo/parse.go | 3 ++ pkg/geo/parse_test.go | 11 +++++++ .../testdata/logic_test/geospatial_bbox | 30 +++++++++++++++++++ pkg/sql/sem/builtins/geo_builtins.go | 20 +++++++++++-- 4 files changed, 61 insertions(+), 3 deletions(-) diff --git a/pkg/geo/parse.go b/pkg/geo/parse.go index 95f229bdb370..6ac60ea56dff 100644 --- a/pkg/geo/parse.go +++ b/pkg/geo/parse.go @@ -239,6 +239,9 @@ func parseGeoHash(g string, precision int) (geohash.Box, error) { return geohash.Box{}, pgerror.Newf(pgcode.InvalidParameterValue, "length of GeoHash must be greater than 0") } + // In PostGIS the parsing is case-insensitive. + g = strings.ToLower(g) + // If precision is more than the length of the geohash // or if precision is less than 0 then set // precision equal to length of geohash. diff --git a/pkg/geo/parse_test.go b/pkg/geo/parse_test.go index a6181f1f20e0..a8c239902f13 100644 --- a/pkg/geo/parse_test.go +++ b/pkg/geo/parse_test.go @@ -642,6 +642,17 @@ func TestParseHash(t *testing.T) { Max: -120.9375, }, }}, + // In PostGIS the parsing is case-insensitive. + {"F", 1, geohash.Box{ + Lat: geohash.Range{ + Min: 45, + Max: 90, + }, + Lon: geohash.Range{ + Min: -90, + Max: -45, + }, + }}, } for _, tc := range testCases { t.Run(fmt.Sprintf("%s[:%d]", tc.h, tc.p), func(t *testing.T) { diff --git a/pkg/sql/logictest/testdata/logic_test/geospatial_bbox b/pkg/sql/logictest/testdata/logic_test/geospatial_bbox index 356a094de17f..4da65154a536 100644 --- a/pkg/sql/logictest/testdata/logic_test/geospatial_bbox +++ b/pkg/sql/logictest/testdata/logic_test/geospatial_bbox @@ -243,3 +243,33 @@ BOX(0 0,0.00000000032741809263825417 0.00000000016370904631912708) BOX(20.012344998976914 -20.012345000286587,20.012345000286587 -20.012344998976914) BOX(90 0,90.00000000032742 0.00000000016370904631912708) BOX(90 0,90.0439453125 0.0439453125) + +query T +SELECT ST_Box2DFromGeoHash('F'::TEXT::TEXT::TEXT, NULL::INT4::INT4)::BOX2D; +---- +BOX(-90 45,-45 90) + +query T +SELECT ST_Box2DFromGeoHash('kkqnpkue9ktbpe5', NULL)::BOX2D; +---- +BOX(20.012344998976914 -20.012345000286587,20.012345000286587 -20.012344998976914) + +query T +SELECT ST_Box2DFromGeoHash('KKQNPKUE9KTBPE5', NULL)::BOX2D; +---- +BOX(20.012344998976914 -20.012345000286587,20.012345000286587 -20.012344998976914) + +query T +SELECT ST_Box2DFromGeoHash('kKqNpKuE9KtBpE5', NULL)::BOX2D; +---- +BOX(20.012344998976914 -20.012345000286587,20.012345000286587 -20.012344998976914) + +query T +SELECT ST_Box2DFromGeoHash(NULL)::BOX2D; +---- +NULL + +query T +SELECT ST_Box2DFromGeoHash(NULL, NULL)::BOX2D; +---- +NULL diff --git a/pkg/sql/sem/builtins/geo_builtins.go b/pkg/sql/sem/builtins/geo_builtins.go index 2c35adc863ad..4229e67d05e3 100644 --- a/pkg/sql/sem/builtins/geo_builtins.go +++ b/pkg/sql/sem/builtins/geo_builtins.go @@ -1185,7 +1185,7 @@ SELECT ST_S2Covering(geography, 's2_max_level=15,s2_level_mod=3'). }, ), "st_box2dfromgeohash": makeBuiltin( - defProps(), + tree.FunctionProperties{NullableArgs: true}, tree.Overload{ Types: tree.ArgTypes{ {"geohash", types.String}, @@ -1193,9 +1193,20 @@ SELECT ST_S2Covering(geography, 's2_max_level=15,s2_level_mod=3'). }, ReturnType: tree.FixedReturnType(types.Box2D), Fn: func(ctx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { + if args[0] == tree.DNull { + return tree.DNull, nil + } + g := tree.MustBeDString(args[0]) - p := tree.MustBeDInt(args[1]) - bbox, err := geo.ParseCartesianBoundingBoxFromGeoHash(string(g), int(p)) + + // Precision is allowed to be NULL, in that case treat it as if the + // argument had not been passed in at all + p := len(string(g)) + if args[1] != tree.DNull { + p = int(tree.MustBeDInt(args[1])) + } + + bbox, err := geo.ParseCartesianBoundingBoxFromGeoHash(string(g), p) if err != nil { return nil, err } @@ -1212,6 +1223,9 @@ SELECT ST_S2Covering(geography, 's2_max_level=15,s2_level_mod=3'). }, ReturnType: tree.FixedReturnType(types.Box2D), Fn: func(ctx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { + if args[0] == tree.DNull { + return tree.DNull, nil + } g := tree.MustBeDString(args[0]) p := len(string(g)) bbox, err := geo.ParseCartesianBoundingBoxFromGeoHash(string(g), p)