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

Expose points_only option through geo_shape field mapper #12893

Merged
merged 1 commit into from Sep 8, 2015
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -36,10 +36,20 @@
*/
public class XShapeCollection<S extends Shape> extends ShapeCollection<S> {

private boolean pointsOnly = false;

public XShapeCollection(List<S> shapes, SpatialContext ctx) {
super(shapes, ctx);
}

public boolean pointsOnly() {
return this.pointsOnly;
}

public void setPointsOnly(boolean pointsOnly) {
this.pointsOnly = pointsOnly;
}

@Override
protected Rectangle computeBoundingBox(Collection<? extends Shape> shapes, SpatialContext ctx) {
Rectangle retBox = shapes.iterator().next().getBoundingBox();
@@ -51,7 +51,9 @@ public Shape build() {
for (Coordinate coord : points) {
shapes.add(SPATIAL_CONTEXT.makePoint(coord.x, coord.y));
}
return new XShapeCollection<>(shapes, SPATIAL_CONTEXT);
XShapeCollection multiPoints = new XShapeCollection<>(shapes, SPATIAL_CONTEXT);
multiPoints.setPointsOnly(true);
return multiPoints;
}

@Override
@@ -27,7 +27,6 @@
import com.vividsolutions.jts.geom.Geometry;
import com.vividsolutions.jts.geom.GeometryFactory;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.logging.ESLogger;
import org.elasticsearch.common.logging.ESLoggerFactory;
import org.elasticsearch.common.unit.DistanceUnit.Distance;
@@ -18,7 +18,9 @@
*/
package org.elasticsearch.index.mapper.geo;

import com.spatial4j.core.shape.Point;
import com.spatial4j.core.shape.Shape;
import com.spatial4j.core.shape.jts.JtsGeometry;
import org.apache.lucene.document.Field;
import org.apache.lucene.index.IndexOptions;
import org.apache.lucene.spatial.prefix.PrefixTreeStrategy;
@@ -38,6 +40,7 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.DistanceUnit;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.index.mapper.FieldMapper;
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.mapper.Mapper;
@@ -85,12 +88,14 @@
public static final String DISTANCE_ERROR_PCT = "distance_error_pct";
public static final String ORIENTATION = "orientation";
public static final String STRATEGY = "strategy";
public static final String STRATEGY_POINTS_ONLY = "points_only";
public static final String COERCE = "coerce";
}

public static class Defaults {
public static final String TREE = Names.TREE_GEOHASH;
public static final String STRATEGY = SpatialStrategy.RECURSIVE.getStrategyName();
public static final boolean POINTS_ONLY = false;
public static final int GEOHASH_LEVELS = GeoUtils.geoHashLevelsForPrecision("50m");
public static final int QUADTREE_LEVELS = GeoUtils.quadTreeLevelsForPrecision("50m");
public static final double LEGACY_DISTANCE_ERROR_PCT = 0.025d;
@@ -143,7 +148,7 @@ public Builder coerce(boolean coerce) {
public GeoShapeFieldMapper build(BuilderContext context) {
GeoShapeFieldType geoShapeFieldType = (GeoShapeFieldType)fieldType;

if (geoShapeFieldType.tree.equals("quadtree") && context.indexCreatedVersion().before(Version.V_2_0_0_beta1)) {
if (geoShapeFieldType.tree.equals(Names.TREE_QUADTREE) && context.indexCreatedVersion().before(Version.V_2_0_0_beta1)) {
geoShapeFieldType.setTree("legacyquadtree");
}

@@ -188,6 +193,9 @@ public GeoShapeFieldMapper build(BuilderContext context) {
} else if (Names.COERCE.equals(fieldName)) {
builder.coerce(nodeBooleanValue(fieldNode));
iterator.remove();
} else if (Names.STRATEGY_POINTS_ONLY.equals(fieldName)) {
builder.fieldType().setPointsOnly(XContentMapValues.nodeBooleanValue(fieldNode));
iterator.remove();
}
}
return builder;
@@ -198,6 +206,7 @@ public GeoShapeFieldMapper build(BuilderContext context) {

private String tree = Defaults.TREE;
private String strategyName = Defaults.STRATEGY;
private boolean pointsOnly = Defaults.POINTS_ONLY;
private int treeLevels = 0;
private double precisionInMeters = -1;
private Double distanceErrorPct;
@@ -215,6 +224,7 @@ protected GeoShapeFieldType(GeoShapeFieldType ref) {
super(ref);
this.tree = ref.tree;
this.strategyName = ref.strategyName;
this.pointsOnly = ref.pointsOnly;
this.treeLevels = ref.treeLevels;
this.precisionInMeters = ref.precisionInMeters;
this.distanceErrorPct = ref.distanceErrorPct;
@@ -236,13 +246,15 @@ public boolean equals(Object o) {
defaultDistanceErrorPct == that.defaultDistanceErrorPct &&
Objects.equals(tree, that.tree) &&
Objects.equals(strategyName, that.strategyName) &&
pointsOnly == that.pointsOnly &&
Objects.equals(distanceErrorPct, that.distanceErrorPct) &&
orientation == that.orientation;
}

@Override
public int hashCode() {
return Objects.hash(super.hashCode(), tree, strategyName, treeLevels, precisionInMeters, distanceErrorPct, defaultDistanceErrorPct, orientation);
return Objects.hash(super.hashCode(), tree, strategyName, pointsOnly, treeLevels, precisionInMeters, distanceErrorPct,
defaultDistanceErrorPct, orientation);
}

@Override
@@ -288,6 +300,10 @@ public void checkCompatibility(MappedFieldType fieldType, List<String> conflicts
conflicts.add("mapper [" + names().fullName() + "] has different [tree]");
}

if ((pointsOnly() != other.pointsOnly())) {
conflicts.add("mapper [" + names().fullName() + "] has different points_only");
}

// TODO we should allow this, but at the moment levels is used to build bookkeeping variables
// in lucene's SpatialPrefixTree implementations, need a patch to correct that first
if (treeLevels() != other.treeLevels()) {
@@ -333,6 +349,14 @@ public void setStrategyName(String strategyName) {
this.strategyName = strategyName;
}

public boolean pointsOnly() {
return pointsOnly;
}

public void setPointsOnly(boolean pointsOnly) {
checkIfFrozen();
this.pointsOnly = pointsOnly;
}
public int treeLevels() {
return treeLevels;
}
@@ -378,6 +402,7 @@ public PrefixTreeStrategy defaultStrategy() {

public PrefixTreeStrategy resolveStrategy(String strategyName) {
if (SpatialStrategy.RECURSIVE.getStrategyName().equals(strategyName)) {
recursiveStrategy.setPointsOnly(pointsOnly());
return recursiveStrategy;
}
if (SpatialStrategy.TERM.getStrategyName().equals(strategyName)) {
@@ -417,6 +442,10 @@ public Mapper parse(ParseContext context) throws IOException {
}
shape = shapeBuilder.build();
}
if (fieldType().defaultStrategy() instanceof RecursivePrefixTreeStrategy && fieldType().pointsOnly() && !(shape instanceof Point)) {
throw new MapperParsingException("[{" + fieldType().names().fullName() + "}] is configured for points only but a " +
((shape instanceof JtsGeometry) ? ((JtsGeometry)shape).getGeom().getGeometryType() : shape.getClass()) + " was found");
}
Field[] fields = fieldType().defaultStrategy().createIndexableFields(shape);
if (fields == null || fields.length == 0) {
return null;
@@ -474,6 +503,9 @@ protected void doXContentBody(XContentBuilder builder, boolean includeDefaults,
if (includeDefaults || fieldType().orientation() != Defaults.ORIENTATION) {
builder.field(Names.ORIENTATION, fieldType().orientation());
}
if (includeDefaults || fieldType().pointsOnly() != GeoShapeFieldMapper.Defaults.POINTS_ONLY) {
builder.field(Names.STRATEGY_POINTS_ONLY, fieldType().pointsOnly());
}
if (includeDefaults || coerce.explicit()) {
builder.field("coerce", coerce.value());
}
@@ -168,6 +168,7 @@ public void testQuadtreeConfiguration() throws IOException {
.field("tree", "quadtree")
.field("tree_levels", "6")
.field("distance_error_pct", "0.5")
.field("points_only", true)
.endObject().endObject()
.endObject().endObject().string();

@@ -181,6 +182,7 @@ public void testQuadtreeConfiguration() throws IOException {
assertThat(strategy.getDistErrPct(), equalTo(0.5));
assertThat(strategy.getGrid(), instanceOf(QuadPrefixTree.class));
assertThat(strategy.getGrid().getMaxLevels(), equalTo(6));
assertThat(strategy.isPointsOnly(), equalTo(true));
}

@Test
@@ -308,7 +310,28 @@ public void testLevelPrecisionConfiguration() throws IOException {
assertThat(strategy.getGrid().getMaxLevels(), equalTo(GeoUtils.quadTreeLevelsForPrecision(70d)+1));
}
}


@Test
public void testPointsOnlyOption() throws IOException {
String mapping = XContentFactory.jsonBuilder().startObject().startObject("type1")
.startObject("properties").startObject("location")
.field("type", "geo_shape")
.field("tree", "geohash")
.field("points_only", true)
.endObject().endObject()
.endObject().endObject().string();

DocumentMapper defaultMapper = createIndex("test").mapperService().documentMapperParser().parse(mapping);
FieldMapper fieldMapper = defaultMapper.mappers().getMapper("location");
assertThat(fieldMapper, instanceOf(GeoShapeFieldMapper.class));

GeoShapeFieldMapper geoShapeFieldMapper = (GeoShapeFieldMapper) fieldMapper;
PrefixTreeStrategy strategy = geoShapeFieldMapper.fieldType().defaultStrategy();

assertThat(strategy.getGrid(), instanceOf(GeohashPrefixTree.class));
assertThat(strategy.isPointsOnly(), equalTo(true));
}

@Test
public void testLevelDefaults() throws IOException {
DocumentMapperParser parser = createIndex("test").mapperService().documentMapperParser();
@@ -45,15 +45,12 @@
import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.elasticsearch.index.query.QueryBuilders.filteredQuery;
import static org.elasticsearch.index.query.QueryBuilders.geoIntersectionQuery;
import static org.elasticsearch.index.query.QueryBuilders.geoIntersectionQuery;
import static org.elasticsearch.index.query.QueryBuilders.geoShapeQuery;
import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.nullValue;
import static org.hamcrest.Matchers.*;

public class GeoShapeIntegrationIT extends ESIntegTestCase {

@@ -479,6 +476,40 @@ public void testOrientationPersistence() throws Exception {
assertThat(orientation, equalTo(ShapeBuilder.Orientation.CCW));
}

@Test
public void testPointsOnly() throws Exception {
String mapping = XContentFactory.jsonBuilder().startObject().startObject("type1")
.startObject("properties").startObject("location")
.field("type", "geo_shape")
.field("tree", randomBoolean() ? "quadtree" : "geohash")
.field("tree_levels", "6")
.field("distance_error_pct", "0.01")
.field("points_only", true)
.endObject().endObject()
.endObject().endObject().string();

assertAcked(prepareCreate("geo_points_only").addMapping("type1", mapping));
ensureGreen();

ShapeBuilder shape = RandomShapeGenerator.createShape(random());
try {
indexRandom(true, client().prepareIndex("geo_points_only", "type1", "1").setSource(jsonBuilder().startObject()
.field("location", shape).endObject()));
} catch (Throwable e) {
// RandomShapeGenerator created something other than a POINT type, verify the correct exception is thrown
assertThat(e.getMessage(), containsString("MapperParsingException"));
assertThat(e.getMessage(), containsString("is configured for points only"));
return;
}

// test that point was inserted
SearchResponse response = client().prepareSearch()
.setQuery(geoIntersectionQuery("location", shape))
.execute().actionGet();

assertEquals(1, response.getHits().getTotalHits());
}

private String findNodeName(String index) {
ClusterState state = client().admin().cluster().prepareState().get().getState();
IndexShardRoutingTable shard = state.getRoutingTable().index(index).shard(0);
@@ -63,6 +63,14 @@ public static ShapeType randomType(Random r) {
}
}

public static ShapeBuilder createShape(Random r) throws InvalidShapeException {
return createShapeNear(r, null);
}

public static ShapeBuilder createShape(Random r, ShapeType st) {
return createShapeNear(r, null, st);
}

public static ShapeBuilder createShapeNear(Random r, Point nearPoint) throws InvalidShapeException {
return createShape(r, nearPoint, null, null);
}
@@ -211,7 +219,8 @@ private static ShapeBuilder createShape(Random r, Point nearPoint, Rectangle wit
try {
pgb.build();
} catch (InvalidShapeException e) {
return null;
// jts bug rarely results in an invalid shape, if it does happen we try again instead of returning null
return createShape(r, nearPoint, within, st, validate);

This comment has been minimized.

Copy link
@dakrone

dakrone Sep 1, 2015

Member

How common is the bug? Are we going to exceed max stack depth here recursing?

This comment has been minimized.

Copy link
@nknize

nknize Sep 4, 2015

Author Member

Not at all common. Typically (~90% of the time) the first recursion creates something other than a polygon (e.g., linestring, multilinestring) thus completes without issue.

}
}
return pgb;
@@ -62,6 +62,16 @@ outer ring vertices in counterclockwise order with inner ring(s) vertices (holes
in clockwise order. Setting this parameter in the geo_shape mapping explicitly
sets vertex order for the coordinate list of a geo_shape field but can be
overridden in each individual GeoJSON document.

|`points_only` |Setting this option to `true` (defaults to `false`) configures
the `geo_shape` field type for point shapes only (NOTE: Multi-Points are not
yet supported). This optimizes index and search performance for the `geohash` and
`quadtree` when it is known that only points will be indexed. At present geo_shape
queries can not be executed on `geo_point` field types. This option bridges the gap
by improving point performance on a `geo_shape` field so that `geo_shape` queries are
optimal on a point only field.


|=======================================================================

[float]
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.