geo/geomfn: implement ST_MinimumBoundingCircle by C0rWin · Pull Request #55567 · cockroachdb/cockroach

C0rWin · 2020-10-15T01:13:41Z

This commit implements ST_MinimumBoundingCircle builtin function to
return polygon shape which approximates minimum bounding circle to
contain provided geometry.

Resolves: #48987

Signed-off-by: Artem Barger bartem@il.ibm.com

Release note (sql change): Returns polygon shape which approximates
minimum bounding circle to contain geometry

blathers-crl · 2020-10-15T01:13:46Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I have added a few people who may be able to assist in reviewing:

@otan (author of geo/geomfn: implement ST_MinimumBoundingCircle({geometry,int4}) #48987)

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

cockroach-teamcity · 2020-10-15T01:13:48Z

This change is

blathers-crl · 2020-10-15T01:22:18Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

C0rWin · 2020-10-15T09:15:39Z

Seems to fail logic tests in CI due to precision issues, are there any suggestions on how to overcome it, @otan ?

PS. Updated UT to work with ST_SnapToGrid.

otan

i'll review this one after you rebase with MinimumBoundingRadius when that merges!

blathers-crl · 2020-10-15T17:46:42Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

C0rWin · 2020-10-15T17:47:37Z

i'll review this one after you rebase with MinimumBoundingRadius when that merges!

Just rebased on top the previous one, though, still need to wait to merge MinimumBoundingRadius first.

blathers-crl · 2020-10-15T19:46:05Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

otan · 2020-10-15T22:02:44Z

pkg/sql/sem/builtins/geo_builtins.go

+				}
+				return tree.NewDGeometry(polygon), nil
+			},
+			Info:       "Returns the smallest circle polygon that can fully contain a geometry. ",


nit: remove trailing space.

otan · 2020-10-15T22:03:10Z

pkg/sql/sem/builtins/geo_builtins.go


+	"st_minimumboundingcircle": makeBuiltin(defProps(),
+		tree.Overload{
+			Types: tree.ArgTypes{


nit: use geometryOverload1 for this

blathers-crl · 2020-10-15T22:41:29Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

otan

Hey -- read the docs over and noticed we're missing a permutation of this argument.

otan · 2020-10-16T15:07:48Z

pkg/sql/sem/builtins/geo_builtins.go


+	"st_minimumboundingcircle": makeBuiltin(defProps(),
+		geometryOverload1(func(evalContext *tree.EvalContext, g *tree.DGeometry) (tree.Datum, error) {
+			polygon, _, _, err := geomfn.MinimumBoundingCircle(g.Geometry)


it looks like we're missing the "num_segs_per_qt_circ" arg here -- see https://postgis.net/docs/ST_MinimumBoundingCircle.html.

I have noted this as well, however, didn't find in GEOS implementation where you can control this. Do you have suggestions?

extern GEOSGeometry GEOS_DLL *GEOSMinimumBoundingCircle_r(GEOSContextHandle_t handle, const GEOSGeometry* g, double* radius, GEOSGeometry** center);

Looks like I've found how to handle this in PostGis

I was going to suggest using ST_Buffer on the center by the radius.

I was going to suggest using ST_Buffer on the center by the radius.

Added implementation to do just this.

otan · 2020-10-16T15:08:21Z

pkg/sql/logictest/testdata/logic_test/geospatial

+query T
+SELECT ST_AsText(ST_SnapToGrid(ST_MinimumBoundingCircle(ST_GeomFromText('GEOMETRYCOLLECTION (LINESTRING(55 75,125 150), POINT(20 80))')), 0.001));
+----
+POLYGON ((135.597000000000008 115, 134.384999999999991 102.689999999999998, 130.794000000000011 90.853999999999999, 124.963000000000008 79.945000000000007, 117.116 70.384, 107.555000000000007 62.536999999999999, 96.646000000000001 56.706000000000003, 84.810000000000002 53.115000000000002, 72.5 51.902999999999999, 60.189999999999998 53.115000000000002, 48.353999999999999 56.706000000000003, 37.445 62.536999999999999, 27.884 70.384, 20.036999999999999 79.945000000000007, 14.206 90.853999999999999, 10.615 102.689999999999998, 9.403 115, 10.615 127.310000000000002, 14.206 139.146000000000015, 20.036999999999999 150.055000000000007, 27.884 159.616000000000014, 37.445 167.462999999999994, 48.353999999999999 173.294000000000011, 60.189999999999998 176.884999999999991, 72.5 178.097000000000008, 84.810000000000002 176.884999999999991, 96.646000000000001 173.294000000000011, 107.555000000000007 167.462999999999994, 117.116 159.616000000000014, 124.963000000000008 150.055000000000007, 130.794000000000011 139.146000000000015, 134.384999999999991 127.310000000000002, 135.597000000000008 115))


can you add a test for the minimum bounding circle of a single point?

otan

thanks for the update! a small stylistic comment and this is good to go :)

otan · 2020-10-19T16:08:13Z

pkg/sql/sem/builtins/geo_builtins.go

+				return nil, err
+			}
+			return tree.NewDGeometry(polygon), nil
+		}, types.Geometry,


nit: new line per argument, i.e.

geometryOverload1( func(evalContext *tree.EvalContext, g *tree.DGeometry) (tree.Datum, error) { }, types,Geometry, infoBuilder{ ... }, }, tree.Overload{ , / ...

otan · 2020-10-19T16:08:40Z

pkg/sql/sem/builtins/geo_builtins.go

+					return nil, err
+				}
+
+				polygon, err := geomfn.Buffer(centroid,


nit: new line per argument, i.e.

polygon, err := geomfn.Buffer( centroid, geomfn.MakeDefaultBufferParams().WithQuadrantSegments(int(numOfSeg)), radius, )

C0rWin · 2020-10-19T21:44:27Z

thanks for the update! a small stylistic comment and this is good to go :)

small update + rebase.

otan

thanks!

bors r+

craig · 2020-10-19T22:24:48Z

Build failed:

GitHub CI (Cockroach)

otan · 2020-10-19T22:25:57Z

looks like a lint failure

This commit implements ST_MinimumBoundingCircle builtin function to return polygon shape which approximates minimum bounding circle to contain provided geometry. Resolves: cockroachdb#48987 Signed-off-by: Artem Barger <bartem@il.ibm.com> Release note (sql change): Returns polygon shape which approximates minimum bounding circle to contain geometry

C0rWin · 2020-10-19T23:33:12Z

looks like a lint failure

apologies missed that one, fixed now

otan

thanks

bors r+

craig · 2020-10-20T16:09:36Z

Build succeeded:

GitHub CI (Cockroach)

blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Oct 15, 2020

blathers-crl bot requested a review from otan October 15, 2020 01:13

C0rWin changed the title ~~geo/geomfn: implement~~ geo/geomfn: implement ST_MinimumBoundingCircle Oct 15, 2020

C0rWin force-pushed the bound_circle branch from 995d5a0 to 748d1ab Compare October 15, 2020 01:22

C0rWin force-pushed the bound_circle branch 2 times, most recently from fca1ea9 to 107acd0 Compare October 15, 2020 08:10

C0rWin force-pushed the bound_circle branch from 107acd0 to 5e9de4b Compare October 15, 2020 11:13

otan reviewed Oct 15, 2020

View reviewed changes

C0rWin force-pushed the bound_circle branch from 5e9de4b to ae5508f Compare October 15, 2020 17:46

blathers-crl bot requested a review from otan October 15, 2020 17:46

C0rWin force-pushed the bound_circle branch from ae5508f to 1cfdd6d Compare October 15, 2020 19:46

otan reviewed Oct 15, 2020

View reviewed changes

C0rWin force-pushed the bound_circle branch from 1cfdd6d to 9d26b9f Compare October 15, 2020 22:41

blathers-crl bot requested a review from otan October 15, 2020 22:41

C0rWin force-pushed the bound_circle branch from 9d26b9f to c34f2e0 Compare October 16, 2020 00:07

otan reviewed Oct 16, 2020

View reviewed changes

C0rWin force-pushed the bound_circle branch from c34f2e0 to 103b549 Compare October 16, 2020 22:34

blathers-crl bot requested a review from otan October 16, 2020 22:34

C0rWin force-pushed the bound_circle branch 2 times, most recently from 867f32e to 2b700e1 Compare October 18, 2020 00:04

otan reviewed Oct 19, 2020

View reviewed changes

C0rWin force-pushed the bound_circle branch from 2b700e1 to 6d616d7 Compare October 19, 2020 21:42

blathers-crl bot requested a review from otan October 19, 2020 21:42

C0rWin force-pushed the bound_circle branch from 6d616d7 to 9665dfb Compare October 19, 2020 21:43

otan approved these changes Oct 19, 2020

View reviewed changes

C0rWin force-pushed the bound_circle branch from 9665dfb to e44afb0 Compare October 19, 2020 23:32

otan approved these changes Oct 20, 2020

View reviewed changes

craig bot merged commit e16811f into cockroachdb:master Oct 20, 2020

Conversation

C0rWin commented Oct 15, 2020

Uh oh!

blathers-crl bot commented Oct 15, 2020

Uh oh!

cockroach-teamcity commented Oct 15, 2020

Uh oh!

blathers-crl bot commented Oct 15, 2020

Uh oh!

C0rWin commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

blathers-crl bot commented Oct 15, 2020

Uh oh!

C0rWin commented Oct 15, 2020

Uh oh!

blathers-crl bot commented Oct 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blathers-crl bot commented Oct 15, 2020

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

C0rWin commented Oct 19, 2020

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

craig bot commented Oct 19, 2020

Uh oh!

otan commented Oct 19, 2020 via email

Uh oh!

C0rWin commented Oct 19, 2020

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

craig bot commented Oct 20, 2020

Uh oh!

Reviewers

Assignees

Labels

C0rWin commented Oct 15, 2020 •

edited

Loading