Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

builtins: add Geography builtins #48529

Merged
merged 1 commit into from
May 13, 2020

Conversation

otan
Copy link
Contributor

@otan otan commented May 7, 2020

Fruits of labour be released to the public!

Added some more helpers for InfoBuilder to include the source library in
which our calculations are done.

Resolves #48367
Resolves #48393
Resolves #48394
Resolves #48395
Resolves #48396
Resolves #48399
Resolves #48400
Resolves #48401

Release note (sql change): We introduce the following newly implemented
functions that work on GEOGRAPHY types:

  • ST_Covers
  • ST_CoveredBy
  • ST_Intersects
  • ST_Distance
  • ST_DWithin
  • ST_Perimeter
  • ST_Area
  • ST_Length

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@otan otan requested review from sumeerbhola and a team May 7, 2020 13:52
@blathers-crl
Copy link

blathers-crl bot commented May 7, 2020

❌ The GitHub CI (Cockroach) build has failed on d0762da0.

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

@otan otan force-pushed the geography_builtins branch 2 times, most recently from bf46bd9 to 8488ecc Compare May 7, 2020 16:37
Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 10 of 10 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @otan)


docs/generated/sql/functions.md, line 668 at r1 (raw file):

<tbody>
<tr><td><a name="st_area"></a><code>st_area(geography: geography) &rarr; <a href="float.html">float</a></code></td><td><span class="funcdesc"><p>Returns the area of the given geography in meters^2. Uses a spheroid to perform the operation.</p>
<p>This function utilizes the S2 library for spherical calculations.</p>

shouldn't this say "utilizes the GeographicLib library for spheroid calculations"?


docs/generated/sql/functions.md, line 748 at r1 (raw file):

<tr><td><a name="st_dwithin"></a><code>st_dwithin(geography_a: geography, geography_b: geography, distance: <a href="float.html">float</a>) &rarr; <a href="bool.html">bool</a></code></td><td><span class="funcdesc"><p>Returns true if any of geography_a is within distance meters of geography_b. Uses a spheroid to perform the operation.</p>
<p>The calculations performed are have a precision of 1cm.</p>
<p>This function utilizes the GeographicLib library for spheroid calculations.</p>

this should also say that it uses S2 for finding the closest points on the sphere.


docs/generated/sql/functions.md, line 820 at r1 (raw file):

</span></td></tr>
<tr><td><a name="st_length"></a><code>st_length(geography: geography) &rarr; <a href="float.html">float</a></code></td><td><span class="funcdesc"><p>Returns the length of the given geography in meters. Uses a spheroid to perform the operation.</p>
<p>This function utilizes the S2 library for spherical calculations.</p>

shouldn't this be "utilizes the GeographicLib library for spheroid calculations"


docs/generated/sql/functions.md, line 915 at r1 (raw file):

</span></td></tr>
<tr><td><a name="st_perimeter"></a><code>st_perimeter(geography: geography) &rarr; <a href="float.html">float</a></code></td><td><span class="funcdesc"><p>Returns the perimeter of the given geography in meters. Uses a spheroid to perform the operation.</p>
<p>This function utilizes the S2 library for spherical calculations.</p>

ditto


pkg/sql/sem/builtins/geo_builtins.go, line 53 at r1 (raw file):

ib.libraryUsage&usesGEOS

Does go fmt remove the space around the &? If not, I'd prefer to have that space since it makes it easier to spot the operator.

Copy link
Contributor Author

@otan otan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @sumeerbhola)


docs/generated/sql/functions.md, line 668 at r1 (raw file):

Previously, sumeerbhola wrote…

shouldn't this say "utilizes the GeographicLib library for spheroid calculations"?

Done.


docs/generated/sql/functions.md, line 748 at r1 (raw file):

Previously, sumeerbhola wrote…

this should also say that it uses S2 for finding the closest points on the sphere.

Done.


docs/generated/sql/functions.md, line 820 at r1 (raw file):

Previously, sumeerbhola wrote…

shouldn't this be "utilizes the GeographicLib library for spheroid calculations"

Done.


docs/generated/sql/functions.md, line 915 at r1 (raw file):

Done.
Done.


pkg/sql/sem/builtins/geo_builtins.go, line 53 at r1 (raw file):

Previously, sumeerbhola wrote…
ib.libraryUsage&usesGEOS

Does go fmt remove the space around the &? If not, I'd prefer to have that space since it makes it easier to spot the operator.

yeah this is a gofmt thing.

@otan otan force-pushed the geography_builtins branch 2 times, most recently from 227271e to 4bd92dc Compare May 11, 2020 21:24
Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 6 of 6 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @otan and @sumeerbhola)


pkg/sql/sem/builtins/geo_builtins.go, line 38 at r2 (raw file):

const usesSpheroidMessage = " Uses a spheroid to perform the operation."
const spheroidDistanceMessage = `"\n\nWhen operating on a spheroid, this function will use the sphere to calculate ` +
	`the closest two points using S2. The spheroid distance is calculated of these two points using GeographicLib. ` +

nit: The spheroid distance between these two points is calculated using GeographicLib

(simpler sentence structure)

@otan otan force-pushed the geography_builtins branch from 4bd92dc to 942d7cd Compare May 13, 2020 02:35
Copy link
Contributor Author

@otan otan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @sumeerbhola)


pkg/sql/sem/builtins/geo_builtins.go, line 38 at r2 (raw file):

Previously, sumeerbhola wrote…

nit: The spheroid distance between these two points is calculated using GeographicLib

(simpler sentence structure)

Done.

@otan otan force-pushed the geography_builtins branch from 942d7cd to c28bcd2 Compare May 13, 2020 02:39
@otan
Copy link
Contributor Author

otan commented May 13, 2020

bors r=sumeerbhola

@craig
Copy link
Contributor

craig bot commented May 13, 2020

Build failed (retrying...)

@blathers-crl
Copy link

blathers-crl bot commented May 13, 2020

❌ The GitHub CI (Cockroach) build has failed on c28bcd2d.

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

Fruits of labour be released to the public!

Added some more helpers for InfoBuilder to include the source library in
which our calculations are done.

Release note (sql change): We introduce the following newly implemented
functions that work on GEOGRAPHY types:
* ST_Covers
* ST_CoveredBy
* ST_Intersects
* ST_Distance
* ST_DWithin
* ST_Perimeter
* ST_Area
* ST_Length
@otan otan force-pushed the geography_builtins branch from c28bcd2 to f4b863d Compare May 13, 2020 03:33
@craig
Copy link
Contributor

craig bot commented May 13, 2020

Canceled

@otan
Copy link
Contributor Author

otan commented May 13, 2020

bors r=sumeerbhola

@craig
Copy link
Contributor

craig bot commented May 13, 2020

Build succeeded

This was referenced May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment