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.

Sign up for GitHub

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

Jump to bottom

geo/geomfn: implement ST_MinimumBoundingCircle({geometry,int4}) #48987

Closed
otan opened this issue May 14, 2020 · 2 comments · Fixed by #55567
Closed

geo/geomfn: implement ST_MinimumBoundingCircle({geometry,int4}) #48987

otan opened this issue May 14, 2020 · 2 comments · Fixed by #55567
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 •
edited
Loading

Implement ST_MinimumBoundingCircle on arguments {geometry,int4}, 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:

Implemented by GEOS.

🤖 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) labels May 14, 2020
@otan otan added the E-easy Easy issue to tackle, requires little or no CockroachDB experience label Jul 17, 2020
@C0rWin C0rWin mentioned this issue Oct 15, 2020
Merged
C0rWin added a commit to C0rWin/cockroach that referenced this issue Oct 15, 2020
@C0rWin
geo/geomfn: implement ST_MinimumBoundingCircle
1cfdd6d 
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 added a commit to C0rWin/cockroach that referenced this issue Oct 16, 2020
@C0rWin
geo/geomfn: implement ST_MinimumBoundingCircle
c34f2e0 
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
Copy link
Contributor

C0rWin commented Oct 17, 2020 •
edited
Loading

While, working on this issue I've noted there is a similar built-in function ST_MaximumInscribedCircle that finds the largest circle that can by fully contained within a geometry. Doesn't it make sense to include it as well? There is the corresponding function in GEOS, hence should not be that hard to add.

Added new issue: #55653

@otan
Copy link
Contributor Author

otan commented Oct 17, 2020 via email

C0rWin added a commit to C0rWin/cockroach that referenced this issue Oct 18, 2020
@C0rWin
geo/geomfn: implement ST_MinimumBoundingCircle
2b700e1 
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
craig bot pushed a commit that referenced this issue Oct 20, 2020
@C0rWin @andreimatei @yuzefovich
Merge #55567 #55604 #55616 #55737
e16811f 
55567: geo/geomfn: implement ST_MinimumBoundingCircle r=otan a=C0rWin

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

55604: kvserver: improve logging tags in multiTestContext r=andreimatei a=andreimatei

Release note: None

55616: colbuilder: disable wrapping of changefeed processors r=yuzefovich a=yuzefovich

The root of the problem is that `Columnarizer` has buffering behavior -
in 20.1 it will be hanging until `coldata.BatchSize()` (1024 by default)
rows are emitted by the changefeed. On 20.2 and master due to dynamic
batch size behavior it will still be hanging but in a slightly
different manner.

This is less of a problem on 20.2 and the current master because the
vectorized engine will not be used for the changefeed DistSQL flow since
the vectorized row count threshold is never met for it (the estimated
row count for the plan is 0, so unless a user does
`SET vectorize_row_count_threshold=0;` or
`SET vectorize=experimental_always;`, we will always use row-by-row
engine). In 20.1 the meaning of `vectorize=on` was different - we never
looked at the threshold and used the vectorized engine if it was
supported.

In order to fix this issue we simply refuse to wrap the changefeed
processors, so the row-by-row engine will be always used for changefeed
flows.

Fixes: #55605.

Release note (bug fix): The current implementation of changefeeds is
incompatible with the vectorized engine, so whenever the latter is being
used to run the former, it could hang indefinitely. This is now fixed.
Namely, on 20.2 releases this could happen if the user runs
`SET vectorize_row_count_threshold=0;`, and on 20.1 releases - if the
user runs `SET vectorize=on`.

55737: schemachange: refactor GC job code r=spaskob a=spaskob

To implement #48775 I will be modifying the schema GC code to
add support for destroying tenants data. I notice3d that the
current code has redundant repetitions which makes it hard to
understand and modify.

The logic for computing new deadline and refreshing tables is
being repeated in 3 code blocks - I have merged them together.

Release note: none.

Co-authored-by: Artem Barger <bartem@il.ibm.com>
Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Spas Bojanov <spas@cockroachlabs.com>
@craig craig bot closed this as completed in e44afb0 Oct 20, 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
Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

geo/geomfn: implement ST_MinimumBoundingCircle C0rWin/cockroach
2 participants
@C0rWin @otan