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
geo/geomfn: implement ST_Snap #61523
Conversation
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: 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! some small things
pkg/geo/geos/geos.h
Outdated
@@ -192,6 +192,9 @@ CR_GEOS_Status CR_GEOS_RelatePattern(CR_GEOS* lib, CR_GEOS_Slice a, CR_GEOS_Slic | |||
CR_GEOS_Status CR_GEOS_VoronoiDiagram(CR_GEOS* lib, CR_GEOS_Slice g, CR_GEOS_Slice env, | |||
double tolerance, int onlyEdges, CR_GEOS_String* ret); | |||
|
|||
// Snap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove this comment
( 51 150, 101 150, 76 175, 51 150 )), | ||
(( 151 100, 151 200, 176 175, 151 100 )))') As poly, | ||
ST_GeomFromText('LINESTRING (5 107, 54 84, 101 100)') As line | ||
) As _; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tihnk this AS _
should bre replaced by tbl (poly, line)
to make it easier to parse.
or use SELECT ST_asText(poly, ...) FROM [SELECT ST_GeomFromText(...)]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would this work?
query T
SELECT ST_AsText(
ST_Snap(poly,line, ST_Distance(poly, line)*1.25)
) AS polysnapped
FROM (SELECT
ST_GeomFromText('MULTIPOLYGON(
(( 26 125, 26 200, 126 200, 126 125, 26 125 ),
( 51 150, 101 150, 76 175, 51 150 )),
(( 151 100, 151 200, 176 175, 151 100 )))') As poly,
ST_GeomFromText('LINESTRING (5 107, 54 84, 101 100)') As line
) tbl(poly, line);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it passes ya
pkg/geo/geomfn/snap.go
Outdated
@@ -0,0 +1,27 @@ | |||
// Copyright 2020 The Cockroach Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: change dates to 2021 here and below
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. |
looks like a missing |
This patch implements ST_Snap. Resolves cockroachdb#49040 Release justification: low risk, high benefit changes to existing functionality Release note (sql change): ST_Snap is now available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors r+
Build succeeded: |
This patch implements ST_Snap.
Resolves #49040
Release justification: low risk, high benefit changes to existing functionality
Release note (sql change): ST_Snap is now available.