geo: implement ST_Summary for Geography/Geometry #49738

hueypark · 2020-05-31T13:55:54Z

Fixes #48405, #49049

Release note (sql change): Implemented the geometry based builtins ST_Summary.

blathers-crl · 2020-05-31T13:55:59Z

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/geogfn: implement ST_Summary({geography}) #48405, author of geo/geomfn: implement ST_Summary({geometry}) #49049)

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

cockroach-teamcity · 2020-05-31T13:56:00Z

This change is

otan

thanks for coming again and doing this!
few minor things :)
nit: title can just be geo: implement ST_Summary for Geography/Geometry

otan · 2020-05-31T22:35:05Z

docs/generated/sql/functions.md

@@ -1051,6 +1051,22 @@ has no relationship with the commit order of concurrent transactions.</p>
 </span></td></tr>
 <tr><td><a name="st_startpoint"></a><code>st_startpoint(geometry: geometry) &rarr; geometry</code></td><td><span class="funcdesc"><p>Returns the first point of a geometry which has shape LineString. Returns NULL if the geometry is not a LineString.</p>
 </span></td></tr>
+<tr><td><a name="st_summary"></a><code>st_summary(geography: geography) &rarr; <a href="string.html">string</a></code></td><td><span class="funcdesc"><p>Returns a text summary of the contents of the geography.</p>
+<p>Flags shown square brackets after the geometry type have the following meaning:
+M: has M coordinate


the generated text here will not look good in HTML.
if you add * in front of these in the info string, these should appear as a HTML list, i.e.

* M: has M coordinate * Z: has Z coordinate * B: has a cached bounding box * G: is geography * S: has spatial reference system

otan · 2020-05-31T22:36:45Z

pkg/geo/summary.go

+// S: has spatial reference system
+func Summary(t geom.T, isGeography bool, offset int) (summary string, err error) {
+ shape, err := geopbShape(t)
+ if err != nil {


nit: we could pass in shape from geometry.Shape() or geography.Shape() and avoid rechecking this again.

otan · 2020-05-31T22:37:16Z

pkg/geo/summary.go

+ return "", err
+ }
+
+ f, err := flag(t, isGeography)


nit: flag name conflicts with package flag and is a little vague. maybe summaryFlag?

otan · 2020-05-31T22:37:39Z

pkg/geo/summary.go

+ numLinearRings := t.NumLinearRings()
+
+ summary += fmt.Sprintf("%s[%s] with %d ring", shape.String(), f, t.NumLinearRings())
+ if 1 < numLinearRings {


nit: easier to read this (and below) as numLinearRings > 1

otan · 2020-05-31T22:41:10Z

pkg/geo/summary.go

+
+ for i := 0; i < numPoints; i++ {
+ point := t.Point(i)
+ tmp, err := Summary(point, isGeography, offset+2)


nit: the variable named tmp here may be better named as line

otan · 2020-05-31T22:43:00Z

pkg/geo/summary.go

+// B: has a cached bounding box
+// G: is geography
+// S: has spatial reference system
+func Summary(t geom.T, isGeography bool, offset int) (summary string, err error) {


nit: taking offset as an initial argument for the public facing function makes the interface a little unfriendly as there is a dangling magic constant 0.

Can we do something like:

func Summary(t geom.T, isGeography bool) (summary string, err error) { return summary(t, isGeography, 0) } func summary(t geom.T, isGeography bool, spaceOffset int) (summary string, err error) { }

blathers-crl · 2020-06-01T12:42:24Z

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.}

hueypark · 2020-06-01T12:43:49Z

Thank you for the review!

nit: title can just be geo: implement ST_Summary for Geography/Geometry

Done.

otan · 2020-06-01T15:46:55Z

pkg/geo/summary.go

+
+ return sum, nil
+ default:
+ return "", errors.Newf("unspport geom type: %T", t)


i've fixed this typo

otan · 2020-06-01T15:47:11Z

pkg/geo/summary.go

+ return summary(t, shape, isGeography, 0)
+}
+
+func summary(t geom.T, shape geopb.Shape, isGeography bool, offset int) (sum string, err error) {


renamed summaryLine

Fixes cockroachdb#48405, cockroachdb#49049 Release note (sql change): Implemented the geometry based builtins `ST_Summary`.

otan · 2020-06-01T16:44:35Z

bors r+

craig · 2020-06-01T17:19:58Z

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 May 31, 2020

blathers-crl bot requested a review from otan May 31, 2020 13:56

otan reviewed May 31, 2020

View reviewed changes

hueypark changed the title ~~geo/geo*fn: implement ST_Summary({geography}), implement ST_Summary({…~~ geo: implement ST_Summary for Geography/Geometry Jun 1, 2020

hueypark force-pushed the st_summary branch from 23b548c to 6f83aa2 Compare June 1, 2020 12:42

blathers-crl bot requested a review from otan June 1, 2020 12:42

otan reviewed Jun 1, 2020

View reviewed changes

geo: implement ST_Summary for Geography/Geometry

bbc956d

Fixes cockroachdb#48405, cockroachdb#49049 Release note (sql change): Implemented the geometry based builtins `ST_Summary`.

otan force-pushed the st_summary branch from 6f83aa2 to bbc956d Compare June 1, 2020 15:49

otan approved these changes Jun 1, 2020

View reviewed changes

craig bot merged commit e43d665 into cockroachdb:master Jun 1, 2020

jseldess mentioned this pull request Jun 24, 2020

geo: implement ST_Summary for Geography/Geometry cockroachdb/docs#7586

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

geo: implement ST_Summary for Geography/Geometry #49738

geo: implement ST_Summary for Geography/Geometry #49738

hueypark commented May 31, 2020 •

edited

Loading

blathers-crl bot commented May 31, 2020

cockroach-teamcity commented May 31, 2020

otan left a comment

otan May 31, 2020

hueypark Jun 1, 2020

otan May 31, 2020

hueypark Jun 1, 2020

otan May 31, 2020

hueypark Jun 1, 2020

otan May 31, 2020

hueypark Jun 1, 2020

otan May 31, 2020

hueypark Jun 1, 2020

otan May 31, 2020

hueypark Jun 1, 2020

blathers-crl bot commented Jun 1, 2020

hueypark commented Jun 1, 2020

otan Jun 1, 2020

otan Jun 1, 2020

otan commented Jun 1, 2020

craig bot commented Jun 1, 2020

geo: implement ST_Summary for Geography/Geometry #49738

geo: implement ST_Summary for Geography/Geometry #49738

Conversation

hueypark commented May 31, 2020 • edited Loading

blathers-crl bot commented May 31, 2020

cockroach-teamcity commented May 31, 2020

otan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blathers-crl bot commented Jun 1, 2020

hueypark commented Jun 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otan commented Jun 1, 2020

craig bot commented Jun 1, 2020

Build succeeded

hueypark commented May 31, 2020 •

edited

Loading