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

geo/geomfn: implement ST_Segmentize({geometry,float8}) #49029

Closed
otan opened this issue May 14, 2020 · 0 comments · Fixed by #49827
Closed

geo/geomfn: implement ST_Segmentize({geometry,float8}) #49029

otan opened this issue May 14, 2020 · 0 comments · Fixed by #49827
Labels
A-geometry-builtins Builtins which have geometry as args. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-easy Easy issue to tackle, requires little or no CockroachDB experience X-nostale Marks an issue/pr that should be ignored by the stale bot

Comments

@otan
Copy link
Contributor

otan commented May 14, 2020

Implement ST_Segmentize on arguments {geometry,float8}, which should adopt PostGIS behaviour.

Observers: Please react to this issue if you need this functionality.

For Geometry builtins, please do the following:

  • Ideally add a relevant helper function in pkg/geo/geomfn (parse and output related functions can go in pkg/geo). Add exhaustive unit tests here - you can run through example test cases and make sure that PostGIS and CRDB return the same result within a degree of accuracy (1cm for geography).
    • When using GEOS, you can reference the C API for which functions are available. Unfortunately, Windows is not currently supported when using GEOS.
  • Create a new builtin that references this function in pkg/sql/sem/builtins/geo_builtins.go. Note that we currently do not support optional arguments, so we define functions that have optional arguments once without the optional argument (using the default value in the optional position), and once with the optional argument.
  • Modify the tests in pkg/sql/logictest/testdata/logic_test/geospatial to call this functionality at least once. You can call make testbaselogic FILES='geospatial' TESTFLAGS='-rewrite' to regenerate the output. Tests here should just ensure the builtin is linked end to end (your exhaustive unit tests go the above mentioned packages!).
  • Ensure the documentation is regenerated by calling make buildshort. You can also play with it by calling ./cockroach demo --empty afterwards.
  • Submit your PR - make sure to follow the guidelines from creating your first PR.

You can follow #48552 for an example PR.

The following additional guidance has been issued on implementing this function:

Move geo/geodist to geo/geocommon, copy the Segmentize function to geo/geocommon abstracting away the spheroid math and make it work for planar math as well.

🤖 This issue was synced with a spreadsheet by gsheets-to-github-issues by otan on 2023-09-03T23:16:38Z. Changes to titles, body and labels may be overwritten.

@otan otan added A-geometry-builtins Builtins which have geometry as args. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-easy Easy issue to tackle, requires little or no CockroachDB experience labels May 14, 2020
abhishek20123g added a commit to abhishek20123g/cockroach that referenced this issue Jun 3, 2020
Fixes cockroachdb#49029

This PR implements ST_Segmentize({geometry, float8}) builtin
function, which allows modify given geometry such that no
segment longer than the given max_segment_length.

Also this PR refactors and add some extra test cases for
ST_Segmentize for geography.

Release note (sql change): This PR implements ST_Segmentize({geometry,
float8}) builtin function.
craig bot pushed a commit that referenced this issue Jun 4, 2020
49827: geo/geomfn: Implements ST_Segmentize for geometry r=otan a=abhishek20123g

Fixes #49029

This PR implements ST_Segmentize({geometry, float8}) builtin
function, which allows modify given geometry such that no
segment longer than the given max_segment_length.

Also this PR refactors and add extra test cases for
ST_Segmentize for geography.

Release note (sql change): This PR implements ST_Segmentize({geometry,
float8}) builtin function.

49833: geo/geomfn: implement Intersection, PointOnSurface, Union r=sumeerbhola a=otan

The last of the topology operators up to Chapter 20.

Resolves #48951
Resolves #49832 
Resolves #49064

Release note (sql change): Implements the ST_Intersection,
ST_PointOnSurface and ST_Union builtin functions.

49869: vendor: bump golang/protobuf to 1.4.2 r=knz a=tbg

v1.4.1 aggressively deprecated something (by inserting panics) that was
reachable via gogoproto's marshaler. Luckily, v1.4.2 has this "fixed";
it caused enough trouble for others as well.

Closes #49842.

Release note: None

49870: schemachange: unskip TestDropWhileBackfill r=spaskob a=spaskob

Disabling the GC job was preventing this test from completing.
Tested with `test stress`: 1000 successful runs.

Fixes #44944.

Release note: none.

49871: kvserver: fixup test failure message r=andreimatei a=andreimatei

Expected and real err were reversed.

Release note: None

Co-authored-by: abhishek20123g <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Tobias Schottdorf <[email protected]>
Co-authored-by: Spas Bojanov <[email protected]>
Co-authored-by: Andrei Matei <[email protected]>
@craig craig bot closed this as completed in 325b615 Jun 4, 2020
@otan otan added the X-nostale Marks an issue/pr that should be ignored by the stale bot label Sep 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-geometry-builtins Builtins which have geometry as args. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-easy Easy issue to tackle, requires little or no CockroachDB experience X-nostale Marks an issue/pr that should be ignored by the stale bot
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant