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

Adds support for GeoJSON GeometryCollection #7123

Merged
merged 1 commit into from Aug 18, 2014

Conversation

Projects
None yet
5 participants
@colings86
Copy link
Member

commented Aug 1, 2014

Closes #2796

@jpountz

View changes

src/main/java/org/elasticsearch/common/geo/builders/GeometryCollectionBuilder.java Outdated
List<Shape> shapes = new ArrayList<>(this.shapes.size());

for (ShapeBuilder shape : this.shapes) {
shapes.add(shape.build());

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 1, 2014

Contributor

looks like there are two levels of indentation instead of one

@pickypg

View changes

docs/reference/mapping/types/geo-shape-type.asciidoc Outdated
@@ -147,7 +147,7 @@ points.
specifying only the top left and bottom right points.
|`N/A` |`circle` |A circle specified by a center point and radius with
units, which default to `METERS`.
|`GeometryCollection` |`N/A` | An unsupported GeoJSON shape similar to the
|`GeometryCollection` |`geometrycollection` | An GeoJSON shape similar to the

This comment has been minimized.

Copy link
@pickypg

pickypg Aug 2, 2014

Member

As minor as it comes: An should be A. Because GeometryCollection is a standard type that is now supported, consider moving it above envelope's listing in this table.

This comment has been minimized.

Copy link
@pickypg

pickypg Aug 2, 2014

Member

You should also add an example at the bottom of how to use the new type before Envelope.

@pickypg

View changes

src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java Outdated
@@ -151,6 +151,14 @@ public static MultiPolygonBuilder newMultiPolygon() {
}

/**
* Create a new GeometryColleciton
* @return a new {@link MultiPolygonBuilder}

This comment has been minimized.

Copy link
@pickypg

pickypg Aug 2, 2014

Member

Typo above (GeometryCollection) and copy/paste error with the return.

@jpountz jpountz removed the review label Aug 4, 2014

@colings86 colings86 self-assigned this Aug 7, 2014

}
]
}
}

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 18, 2014

Contributor

Feels inconsistent that geometrycollection is all lowercase while Point and LineString are camelcase?

@jpountz

View changes

src/main/java/org/elasticsearch/common/geo/builders/GeometryCollectionBuilder.java Outdated
import java.util.ArrayList;
import java.util.List;

public class GeometryCollectionBuilder extends ShapeBuilder{

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 18, 2014

Contributor

Missing space before the bracket

@jpountz

View changes

src/main/java/org/elasticsearch/common/geo/builders/GeometryCollectionBuilder.java Outdated
builder.startObject();
builder.field(FIELD_TYPE, TYPE.shapename);
builder.startArray(FIELD_GEOMETRIES);
for(ShapeBuilder shape : shapes) {

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 18, 2014

Contributor

Missing space after the for

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2014

LGTM. I left some comments about cosmetics but feel free to push without further review.

@jpountz jpountz removed the review label Aug 18, 2014

@spinscale

This comment has been minimized.

Copy link
Member

commented Aug 18, 2014

This PR only tests the parsing, does it make sense to test a real query as well, to make sure that having different shapes works as expected (I'd guess so though)?

@colings86

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2014

@spinscale Added the test, could you just confirm that this is what you meant before I push?

@colings86 colings86 added the review label Aug 18, 2014

@spinscale

View changes

src/test/java/org/elasticsearch/search/geo/GeoShapeIntegrationTests.java Outdated
.endObject()
.endArray()
.endObject().endObject();
System.out.println(docSource.humanReadable(true).string());

This comment has been minimized.

Copy link
@spinscale
@spinscale

View changes

src/test/java/org/elasticsearch/search/geo/GeoShapeIntegrationTests.java Outdated
result = client().prepareSearch("test").setQuery(QueryBuilders.matchAllQuery())
.setPostFilter(filter).get();
assertSearchResponse(result);
assertHitCount(result, 0);filter = FilterBuilders.geoShapeFilter("location", ShapeBuilder.newGeometryCollection()

This comment has been minimized.

Copy link
@spinscale

spinscale Aug 18, 2014

Member

add a newline here

@spinscale

This comment has been minimized.

Copy link
Member

commented Aug 18, 2014

@colings86 yeah, that was what I meant

@colings86 colings86 merged commit b228691 into elastic:master Aug 18, 2014

@colings86 colings86 deleted the colings86:feature/geoCollectionSupport branch Aug 18, 2014

@colings86 colings86 removed the review label Aug 18, 2014

@colings86 colings86 assigned colings86 and unassigned colings86 Aug 21, 2014

@clintongormley clintongormley changed the title Geo: Adds support for GeoJSON GeometryCollection Adds support for GeoJSON GeometryCollection Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.