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

Closed
otan opened this issue Feb 21, 2021 · 0 comments · Fixed by #61286
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_Affine on arguments {geometry,float8,float8,float8,float8,float8,float8,float8,float8,float8,float8,float8,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 function

🤖 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
craig bot pushed a commit that referenced this issue Mar 2, 2021
61201: sql: populate database_name when selecting from crdb_internal.create_statements r=rafiss a=RichardJCai

Previously, SELECTING from crdb_internal.create_statements and not
using the virtual index would result in database_name being nil.
This was because we did not pass the db descriptor into populate.

Release justification: Low risk, bug fix, internal table
Release note (sql change): selecting from crdb_internal.create_statements
will now correctly populate the database_name when the virtual index is
not used. Internal table.

Fixes #59561

61274: bazel: pin c++ toolchain to clang 10.0 for dev work r=rickystewart a=rickystewart

This can be enabled with:

    build --extra_toolchains=@llvm_toolchain//:cc-toolchain-darwin

in `.bazelrc.user`.

Use the open-source `grailbio/bazel-toolchain` library to pin to clang
v10.0. I selected this library because it appears to be the most
complete Clang toolchain in the open-source world for Bazel, and
it didn't seem like writing my own thing from scratch would provide any
benefits over what already exists here.

The open-source library is incomplete in several important ways, namely
that the toolchain detection is based on which OS you're using, so
remote execution builds and cross-compiled builds are very likely to not
work. We have to solve both of these problems anyway, but that's not
urgent. As appropriate, we can fork and submit pull requests for the
upstream repo, or if it's more sensible to write our own custom
toolchain specifically for cross-compilation, we can do so.

Fixes #56054
Fixes #56055
Fixes #56056
Fixes #56057

Release note: None

Release justification: Non-production code changes

61286: builtins: implement ST_Affine for 3D transformations r=otan a=alsohas

Resolves #60874.

Release justification: low risk addition to new functionality
Release note (sql change): The geography builtin ST_Affine now supports
3D transformations

Co-authored-by: richardjcai <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Abdullah Islam <[email protected]>
@craig craig bot closed this as completed in d5fbfd7 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
1 participant