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

Fix/geoshape #788

Merged
merged 15 commits into from
Jul 16, 2014
Merged

Fix/geoshape #788

merged 15 commits into from
Jul 16, 2014

Conversation

gmarz
Copy link
Contributor

@gmarz gmarz commented Jul 14, 2014

This PR addresses issues two issues (#776 and #782) with the current geo shape support in NEST. Previously, NEST only allowed for a Coordinates object of type IEnumerable<IEnumerable> for shapes which only worked for a sub set of the GeoJSON objects that Elasticsearch supports.

GeoShape descriptors now expose a Shape() method rather than Type() and Coordinates(), which accepts an IGeometryObject. All of the supported GeoJSON objects can be found in Nest/Domain/Geometry and all inherit from a base GeometryObject which implements IGeometryObject.

Instead of this:

.Query(q=>q
    .GeoShape(qs=>qs
        .OnField(p=>p.MyGeoShape)
        .Coordinates(new [] { new [] {13.0, 53.0}, new [] { 14.0, 52.0} })
        .Type("envelope")
    )

One can now do this:

.Query(q => q
    .GeoShape(qs => qs
        .OnField(p => p.MyGeoShape)
        .Shape(new Envelope { Coordinates = new[] { new[] { 13.0, 53.0 }, new[] { 14.0, 52.0 } } })
    )

Which automatically sets the geo shape type as well. There is no longer a need to call Type() to specifiy it separately anymore.

When using the OIS, ToGeoShape() has to explicitly be called, whereas the descriptors call it automatically when Shape is set:

    var envelope = new Envelope 
    { 
        Coordinates = new[] { new[] { 13.0, 53.0 }, new[] { 14.0, 52.0 }
    };

    QueryContainer query = new GeoShapeQuery
    {
        Field = "myGeoShape",
        Shape = envelope.ToGeoShape()
    };

Not very ideal, open to suggestions here. The reason for this is that the GeoShape object that's part of the request needs to be generic enough to account for all the different geo shape parameters ES takes. Right now, the main differences are the different Coordinate types (which is why Coordinates is of type object), and Radius which only applies when using the Circle shape.

Check out the tests here to see an example usage of all the shapes: https://github.com/elasticsearch/elasticsearch-net/tree/fix/geoshape/src/Tests/Nest.Tests.Unit/Search/Query/Singles/GeoShape

NOTE: This introduces a breaking change between RC1.0 to GA, but one worth having. We could put back the Type() and Coordinates() methods and just let Shape() override them, but it may be confusing.

@Mpdreamz
Copy link
Member

Hey @gmarz

Some initial thoughts:

Im not totally sold on the IGeometryObject<TCoordinates> construct. The type parameter is unbounded and the ToGeoShape() feels a little foreign.

I think the simpler solution would be to simplify it to:

public interface IGeoShape
{
    [JsonProperty("type")]
    string Type { get; set; }
}

Then each individual shape is free to define Coordinates as they see fit or not at all and map other properties instead such as Radius.

Then the new contract will be .ShapeShape(IGeoShape shape) and folks can more easily define their own subclasses of IGeoShape in case we miss one or a plugin offers more shape types.

For this to work the interface will need to be added to the list in CreateProperties() in NestContractResolver

@gmarz
Copy link
Contributor Author

gmarz commented Jul 14, 2014

@Mpdreamz So that is the exact route I initially went down, and it works as expected except for one issue:

With the new object initializer syntax, we want to be able to deserialize any string into a NEST query object (https://github.com/elasticsearch/elasticsearch-net/tree/develop/src/Tests/Nest.Tests.Unit/QueryParsers). With just an IGeoShape object as part of the request, that only has a Type property, Coordinates and Radius are both lost after deserializing, because it only knows about IGeoShape, thus failing https://github.com/elasticsearch/elasticsearch-net/blob/develop/src/Tests/Nest.Tests.Unit/QueryParsers/Queries/GeoShapeQueryTests.cs.

Otherwise, that would be the preferred solution. What do you think?

@Mpdreamz
Copy link
Member

We might need specialized sub interfaces of IGeoShape in that case much like we needed for IFuzzyQuery

https://github.com/elasticsearch/elasticsearch-net/blob/develop/src/Tests/Nest.Tests.Unit/QueryParsers/Queries/FuzzyQueryTests.cs#L59

In combination with a custom deserializer.

Or we give up and simplify the interface to include coordinates and radius always.

@gmarz
Copy link
Contributor Author

gmarz commented Jul 15, 2014

@Mpdreamz the IGeoShape and sub-interfaces approach you suggested worked like a charm. This PR is complete now and ready to be merged.

Fluent example:

.Query(q => q
    .GeoShapeCircle(c => c
        .OnField(p => p.MyGeoShape)
        .Coordinates(new[] { -45.0, 45.0 })
        .Radius("100m")
    )

Object initializer example:

FilterContainer filter = new GeoShapeCircleFilter
{
    Shape = new CircleGeoShape
    {
        Coordinates = new[] { -45.0, 45.0 },
        Radius = "100m"
    }
};

Conflicts:
	src/Nest/DSL/Filter/GeoShapeFilterDescriptor.cs
	src/Nest/Domain/DSL/GeoShapeVector.cs
	src/Nest/Nest.csproj
@Mpdreamz Mpdreamz merged commit ba869b6 into develop Jul 16, 2014
@gmarz gmarz deleted the fix/geoshape branch July 18, 2014 12:44
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.

2 participants