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

Feature/set circle support #745

Closed

Conversation

undeadcat
Copy link
Contributor

@undeadcat undeadcat commented Jun 10, 2024

Please do not open a pull request without first filing an issue and/or discussing the feature directly with the project maintainer.

Please ensure you adhere to every item in this list

Describe your changes

Adds support for SET ... CIRCLE.

127.0.0.1:9851> SET cities sf POINT 37.769092 -122.451505
OK
127.0.0.1:9851> SET cities 10_km_from_sf CIRCLE 37.769092 -122.451505 10000
OK
127.0.0.1:9851> SET cities 100_km_from_sf CIRCLE 37.769092 -122.451505 100000
OK
127.0.0.1:9851> GET cities 10_km_from_sf
"{\"type\":\"Feature\",\"geometry\":{\"type\":\"Point\",\"coordinates\":[-122.451505,37.769092]},\"properties\":{\"type\":\"Circle\",\"radius\":10000,\"radius_units\":\"m\"}}"
127.0.0.1:9851> INTERSECTS cities POINT 37.769092 -122.451505
1) (integer) 0
2) 1) 1) "100_km_from_sf"
      2) "{\"type\":\"Feature\",\"geometry\":{\"type\":\"Point\",\"coordinates\":[-122.451505,37.769092]},\"properties\":{\"type\":\"Circle\",\"radius\":100000,\"radius_units\":\"m\"}}"
   2) 1) "10_km_from_sf"
      2) "{\"type\":\"Feature\",\"geometry\":{\"type\":\"Point\",\"coordinates\":[-122.451505,37.769092]},\"properties\":{\"type\":\"Circle\",\"radius\":10000,\"radius_units\":\"m\"}}"
   3) 1) "sf"
      2) "{\"type\":\"Point\",\"coordinates\":[-122.451505,37.769092]}"

127.0.0.1:9851> INTERSECTS cities IDS POINT 37.769092 -122.451505
1) (integer) 0
2) 1) "100_km_from_sf"
   2) "10_km_from_sf"
   3) "sf"

127.0.0.1:9851> INTERSECTS cities IDS POINT 37.554478 -122.020158
1) (integer) 0
2) 1) "100_km_from_sf"

This is based on PR #649, but with some differences:

  • feat: allow SET with CIRCLE #649 no longer compiles
  • this uses tidwall/geojsons Circle type, while the previous PR stored the polygon representation of the circle.

Issue number and link

Pull request require a prior issue with discussion.
Include the issue number of link here.

test for containing concentric circles fails, seems bug in geojson library
@iwpnd
Copy link
Contributor

iwpnd commented Jun 10, 2024

Duplicate of #649

@@ -100,8 +100,8 @@ func (o *Object) String() string {
}

func (o *Object) IsSpatial() bool {
_, ok := o.geo().(geojson.Spatial)
return ok
_, ok := o.geo().(String)
Copy link
Contributor Author

@undeadcat undeadcat Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change (and consequently, the move of this file to collection package) was due to:

  • Circle does not implement geojson.Spatial interface, yet we do want to store it in the geo tree
  • geojson.Spatial does not seem to be used as an interface to call methods in this project, only as a 'marker interface'

I went with explicitly marking that we are not interested in strings here instead of using Spatial.

Alternatively, could possibly implement geojson.Spatial in Circle, open to other ideas as welll...

@tidwall
Copy link
Owner

tidwall commented Jun 17, 2024

The problem with a circle type as a geometry stored in a collection is that there is no GeoJSON compatible way to represent it without losing precision on output. I'm not opposed with the idea of a SET ... CIRCLE ... command but I would like to somehow solve the compatibility issue.

@undeadcat undeadcat closed this Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants