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

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

geo/geomfn: implement ST_RotateX({geometry,float8}) #60893

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

Implement ST_RotateX 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 pushed a commit to busiest-beaver/cockroach that referenced this issue Mar 2, 2021
This patch implements ST_RotateX.

Resolves cockroachdb#60893.

Release justification: low-risk updates to new functionality
Release note (sql change): ST_RotateX function is now available.
craig bot pushed a commit that referenced this issue Mar 2, 2021
61288: sql: bump array type alias version on enum value add/drop r=arulajmani a=arulajmani

Previously, when a value was added/dropped from a user defined type,
the corresponding type alias' version was not bumped. This is
problematic because the changes to the underlying enum type descriptor
wouldn't be picked up by the type alias (until the lease expired and
the descriptor was leased again). This behavior may seem slightly
surprising, but it has to do with the fact that the array type alias
stores a `types.T` inside the descriptor. This `types.T` is hydrated
only once and subsequent accesses don't rehydrate it (even if the
underlying enum type descriptor, that hydrated it to begin with, has had
its version bumped). Put another way, for all intents and purposes, we
were violating the two descriptor version invariant for array type
aliases in a bunch of places before this patch.

Fixes #58710

Release justification: bug fixes in existing functionality that affects
multiregion.
Release note (bug fix): `ALTER TYPE ... ADD VALUE` changes are picked
up by the array type alias correctly.

61307: parser: alias REGIONAL / REGIONAL PARTITIONED / PARTITIONED REGIONAL r=ajstorm a=otan

Add REGIONAL, REGIONAL PARTITIONED and PARTITIONED REGIONAL as aliases
for REGIONAL BY TABLE, REGIONAL BY ROW and REGIONAL BY ROW respectively.

Release justification: low-risk update to new functionality

Release note: None

61326: geo/geomfn: implement ST_RotateX r=otan a=alsohas

This patch implements `ST_RotateX`.

Resolves #60893.

Release justification: low-risk updates to new functionality
Release note (sql change): `ST_RotateX` function is now available.

61351: *: use GOMAXPROCS instead of NumCPU r=RaduBerinde a=RaduBerinde

Change NumCPU to GOMAXPROCS when we are using it to set the
parallelism of some subsystem. In most cases these are the same, but
GOMAXPROCS may be intentionally limited for race-detector test runs as
well as containerized environments.

The change also adds a linter which requires using the `system.NumCPU`
wrapper which includes some commentary.

Release justification: low risk, high benefit changes to existing
functionality; attempt at addressing test race failures.

Informs #61120.

Release note: None

Co-authored-by: arulajmani <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Abdullah Islam <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
@craig craig bot closed this as completed in ab2728b Mar 2, 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
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