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_Azimuth({geometry,geometry}) #48887

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

geo/geomfn: implement ST_Azimuth({geometry,geometry}) #48887

otan opened this issue May 14, 2020 · 0 comments · Fixed by #50188
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_Azimuth on arguments {geometry,geometry}, 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.

🤖 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
hueypark added a commit to hueypark/cockroach that referenced this issue Jun 14, 2020
@hueypark
geo/geogfn: implement ST_Azimuth({geometry,geometry})
f56dc8e 
Fixes cockroachdb#48887

Release note (sql change): This PR implement adds the ST_Azimuth({geometry,geometry})
@hueypark hueypark mentioned this issue Jun 14, 2020
Merged
hueypark added a commit to hueypark/cockroach that referenced this issue Jun 14, 2020
@hueypark
geo/geogfn: implement ST_Azimuth({geometry,geometry})
f5c0cce 
Fixes cockroachdb#48887

Release note (sql change): This PR implement adds the ST_Azimuth({geometry,geometry})
craig bot pushed a commit that referenced this issue Jun 15, 2020
@rohany @yuzefovich
@hueypark @knz @petermattis @jbowens
Merge #49284 #50001 #50188 #50205 #50239 #50243
5cff000 
49284: opt: synthesize check constraints on enum columns r=rohany a=rohany

Fixes #49263.

This PR teaches the optimizer how to synthesize check constraints on
columns of an ENUM type, allowing queries like:

```
CREATE TYPE t AS ENUM ('howdy', 'hello');
CREATE TABLE tt (x t, y INT, PRIMARY KEY (x, y));
SELECT x, y FROM tt WHERE y = 2
```

to be planned using constrained spans on the enum values, rather than a
full table scan.

Release note (performance improvement): Allow the optimizer to use enum
information to generate better query plans.

50001: colexec: minor miscellaneous additions r=yuzefovich a=yuzefovich

**colexec: add casts from datum-backed types to bools**

While investigating unrelated test failures, I added this cast, so we
might as well merge it.

Addresses: #48135.

Release note: None

**colexec: add support for Values core with zero rows**

Release note: None

50188: geo/geogfn: implement ST_Azimuth({geometry,geometry}) r=otan a=hueypark

Fixes #48887

Release note (sql change): This PR implement adds the ST_Azimuth({geometry,geometry})

50205: Makefile: ensure redact_safe.md generation is stable r=RichardJCai a=knz

Fixes #50146

On some platforms, the default behavior of `git grep` is to number
the lines (i.e. `-n` is implicitly added).

This patch makes the `-n` explicit and tweaks the sed patterns to
ensure that the behavior is stable.

Release note: None

50239: kv/kvserver: disable the below-raft proto tracking in race builds r=petermattis a=petermattis

The below-raft proto tracking is meant to catch bugs where we
inadvertently starting marshaling a proto below Raft. This tracking is a
source of signficant memory allocations which measurably slow down race
builds. Since we're already doing this tracking in non-race builds,
there is little benefit to also doing it in race builds. Disabling
below-raft proto tracking for race builds reduces the time to run
`testrace` on the `kv/kvserver` package from 605s to 517s on my laptop.

Release note: None

50243: kv/kvserver: fix TestSideloadingSideloadedStorage w/ RocksDB r=jbowens a=jbowens

Fix the TestSideloadingSideloadedStorage test when running with
COCKROACH_STORAGE_ENGINE=rocksdb. The test has always depended on the
exact error message surfaced from os.Remove. In #49717,
diskSideloadStorage was modified to use the engine's RemoveDir operation
rather than interacting with the filesystem directly. Since Pebble uses
os.Remove for its implementation and emulates its error messages in its
MemFS implementation, the exact message comparison continued to succeed.

After #49717, when running with RocksDB, the RocksDB env's error message
was surfaced, with its own context prefixing. This updates the test to
case-insensitively check for 'directory not empty' instead.

Fixes #50226.

Release note: None

Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Jaewan Park <jaewan.huey.park@gmail.com>
Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Co-authored-by: Peter Mattis <petermattis@gmail.com>
Co-authored-by: Jackson Owens <jackson@cockroachlabs.com>
@craig craig bot closed this as completed in 24585a9 Jun 15, 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/geogfn: implement ST_Azimuth({geometry,geometry}) hueypark/cockroach
1 participant
@otan