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

Conversation

Projects
None yet
2 participants
@gmarz
Member

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

This comment has been minimized.

Show comment
Hide comment
@Mpdreamz

Mpdreamz Jul 14, 2014

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

Member

Mpdreamz commented Jul 14, 2014

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

This comment has been minimized.

Show comment
Hide comment
@gmarz

gmarz Jul 14, 2014

Member

@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?

Member

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

This comment has been minimized.

Show comment
Hide comment
@Mpdreamz

Mpdreamz Jul 14, 2014

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.

Member

Mpdreamz commented Jul 14, 2014

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

This comment has been minimized.

Show comment
Hide comment
@gmarz

gmarz Jul 15, 2014

Member

@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"
    }
};
Member

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"
    }
};
Merge branch 'develop' into fix/geoshape
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

This comment has been minimized.

Show comment
Hide comment
@gmarz

gmarz Jul 16, 2014

Member

Closes #782

Member

gmarz commented on 6294d32 Jul 16, 2014

Closes #782

@gmarz gmarz deleted the fix/geoshape branch Jul 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment