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_RotateY({geometry,float8}) #60894

Closed
otan opened this issue Feb 21, 2021 · 0 comments · Fixed by #61387
Closed

geo/geomfn: implement ST_RotateY({geometry,float8}) #60894

otan opened this issue Feb 21, 2021 · 0 comments · Fixed by #61387
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 Feb 21, 2021 •
edited
Loading

Implement ST_RotateY 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:

use the existing affine transformation

🤖 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 Feb 21, 2021
@busiest-beaver busiest-beaver mentioned this issue Mar 3, 2021
Merged
craig bot pushed a commit that referenced this issue Mar 3, 2021
@otan @fqazi
Merge #61282 #61361 #61387
720a144 
61282: sql: block hash sharded indexes for REGIONAL BY ROW r=ajstorm a=otan

This is not supported, so block entry points where a REGIONAL BY ROW
table becomes hash sharded.

Release justification: low risk change to new functionality

Release note: None

61361: sql: create database if not exists see's dropped database's as existing r=fqazi a=fqazi

Fixes: #61150

Previously, the CREATE DATABASE IF NOT EXISTS statement would detect
existing databases, but never checked if they were in a dropping state.
So, the creation process would be skipped and the drop would still
continue leading to unexpected behaviour (i.e. the new database to
replace it would not get created). To address this, this patch
returns an error if a database is found in a dropping state.

Release note (bug fix): Dropping and recreating a database
in a transaction will now correctly error out if an object
in a dropped state is detected.

Release justification: Low risk change to address a high severity
issue where users could have failures due to incorrect behaviour.

61387: geo/geomfn: implement ST_RotateY and ST_RotateZ r=otan a=alsohas

This patch implements ST_RotateY and ST_RotateZ.

Resolves #60894
Resolves #60895

Release justification: low-risk updates to new functionality
Release note (sql change): ST_RotateY and ST_RotateZ are now available.

Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Abdullah Islam <[email protected]>
@craig craig bot closed this as completed in 66050f2 Mar 3, 2021
@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_RotateY and ST_RotateZ busiest-beaver/cockroach
1 participant
@otan