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: coerce invalid geography coordinates into geometry #50219

Closed
otan opened this issue Jun 15, 2020 · 3 comments · Fixed by #50457
Closed

geo: coerce invalid geography coordinates into geometry #50219

otan opened this issue Jun 15, 2020 · 3 comments · Fixed by #50457
Labels
A-spatial Spatial work that is *not* related to builtins. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) good first issue

Comments

@otan
Copy link
Contributor

otan commented Jun 15, 2020

in PostGIS, converting out of range lat-lngs converts it to the correct range, e.g.

otan=# select st_asewkt('point(200 200)'::geography);
NOTICE:  Coordinate values were coerced into range [-180 -90, 180 90] for GEOGRAPHY
LINE 1: select st_asewkt('point(200 200)'::geography);
                         ^
         st_asewkt
---------------------------
 SRID=4326;POINT(-160 -20)
(1 row)

otan=# select st_asewkt('point(200 200)'::geometry::geography);
NOTICE:  Coordinate values were coerced into range [-180 -90, 180 90] for GEOGRAPHY
         st_asewkt
---------------------------
 SRID=4326;POINT(-160 -20)
(1 row)

We should emulate this behaviour.
@blathers-crl
Copy link

blathers-crl bot commented Jun 15, 2020

Hi @otan, please add a C-ategory label to your issue. Check out the label system docs.

While you're here, please consider adding an A- label to help keep our repository tidy.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@otan otan added A-spatial Spatial work that is *not* related to builtins. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) good first issue labels Jun 15, 2020
@sooryranga
Copy link
Contributor

Would love to take a look if no one else is working on it!

@otan
Copy link
Contributor Author

otan commented Jun 19, 2020 •
edited
Loading

go for it! you don't have to worry about the NOTICE, if that helps.

sooryranga added a commit to sooryranga/cockroach that referenced this issue Jun 21, 2020
@sooryranga
geo: coerce invalid geography coordinates into geometry
258a664 
This commit resolves: cockroachdb#50219

Previously geometry did not convert out of range lat-lngs into
correct range. This commit mimics the behavior found in PostGIS.
It converts out of range lat-lngs into correct range.

Release note (sql change): Geometry now coerce invalid
geography coordinates into correct geometry.
@sooryranga sooryranga mentioned this issue Jun 21, 2020
Merged
@otan otan mentioned this issue Jun 29, 2020
Merged
sooryranga added a commit to sooryranga/cockroach that referenced this issue Jul 1, 2020
@sooryranga
geo: coerce invalid geography coordinates into geometry
a22c16f 
This commit resolves: cockroachdb#50219

Previously geometry did not convert out of range lat-lngs into
correct range. This commit mimics the behavior found in PostGIS.
It converts out of range lat-lngs into correct range.

Release note (sql change): Geometry now coerce invalid
geography coordinates into correct geometry.
craig bot pushed a commit that referenced this issue Jul 1, 2020
@sooryranga @yuzefovich
Merge #50457 #50560
e99e7a8 
50457: geo: coerce invalid geography coordinates into geometry r=otan a=Arun4rangan

This commit resolves: #50219

Previously geometry did not convert out of range lat-lngs into
correct range. This commit mimics the behavior found in PostGIS.
It converts out of range lat-lngs into correct range.

Release note (sql change): Geometry now coerce invalid
geography coordinates into correct geometry.

50560: sql: add support for aggregations and values in the new factory r=yuzefovich a=yuzefovich

**sql: remove some unnecessary things around aggregations**

Release note: None

**sql: add support for aggregations in the new factory**

This commit implements `ConstructGroupBy` and `ConstructScalarGroupBy`
methods in the new factory by populating the specs upfront and reusing
DistSQLPlanner to do the actual planning.

Fixes: #50290.

Release note: None

**sql: minor cleanup of planNodeToRowSource**

This commit cleans up `planNodeToRowSource` to be properly closed
(similar to other processors).

Release note: None

**sql: implement ConstructValues in the new factory**

This commit adds implementation of `ConstructValues` in the new factory.
In some cases, a valuesNode is the only way to "construct values" - when
the node must be wrapped (see createPhysPlanForValuesNode for more
details). In other cases, a valuesNode is used to construct the values
processor spec. The latter usage is refactored to avoid valuesNode
creation.

This decision also prompts us to add a separate "side" list of
`planNode`s that are part of the physical plan and need to be closed
when the whole plan is closed. This is done by introducing a utility
wrapper around `PhysicalPlan`. This approach was chosen after
considering an alternative in which `planNodeToRowSource` adapter would
be the one closing the `planNode` it is wrapping because
`planNode.Close` contract currently prohibits the execution from closing
any of the `planNode`s, and changined that would be quite invasive at
this point. We might reevaluate this decision later, once we've made
more progress on the distsql spec work.

Addresses: #47473.

Release note: None

**logictest: add spec-planning configs to the default ones**

We have now implemented noticeable chunk of methods in the new factory,
so I think it makes to run all logic tests with the spec-planning
configs by default.

The only logic test that is currently skipped is `interleaved_join`
because the new factory doesn't plan interleaved joins yet.

Release note: None

**sql: enhance unsupported error message in the new factory**

Release note: None

**sql: support simple projection on top of planNode in the new factory**

The new factory still constructs some of the `planNode`s (for example,
explain variants), and we should be able to handle simple projections on
top of those. This is handled by reusing the logic from the old factory.
The issue was hidden by the fallback to the old factory since EXPLAIN
statements don't have "SELECT" statement tag.

Release note: None

**sql: implement ConstructLimit in the new factory**

This commit additionally removes the requirement that `limitExpr` and
`offsetExpr` must be distributable in order for the whole plan to be
distributable in the old path because those expressions are evaluated
during the physical planning, locally.

Release note: None

Co-authored-by: Arun Ranganathan <arun.ranga@hotmail.ca>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
@craig craig bot closed this as completed in 1184c63 Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-spatial Spatial work that is *not* related to builtins. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) good first issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

geo: coerce invalid geography coordinates into geometry sooryranga/cockroach
2 participants
@otan @sooryranga