From f44822ae8302c8f5859d182082cfd6b4ef328a75 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 19 Apr 2021 12:38:01 +0100 Subject: [PATCH] Rework geo mappers to index value by value (#71696) The various geo field mappers are organised in a hierarchy that shares parsing and indexing code. This ends up over-complicating things, particularly when we have some mappers that accept multiple values and others that only accept singletons. It also leads to confusing behaviour around ignore_malformed behaviour: geo fields will ignore all values if a single one is badly formed, while all other field mappers will only ignore the problem value and index the rest. Finally, this structure makes adding index-time scripts to geo_point needlessly complex. This commit refactors the indexing logic of the hierarchy to move the individual value indexing logic into the concrete implementations, and aligns the ignore_malformed behaviour with that of other mappers. It contains two breaking changes: * The geo field mappers no longer check for external field values on the parse context. This added considerable complication to the refactored parse methods, and is unused anywhere in our codebase, but may impact plugin-based field mappers which expect to use geo fields as multifields * The geo_point field mapper now passes geohashes to its multifields one-by-one, instead of formatting them into a comma-delimited string and passing them all at once. Completion multifields using this as an input should still behave as normal because by default they would split this combined geohash string on the commas in any case, but keyword subfields may look different. Fixes #69601 --- .../xcontent/support/MapXContentParser.java | 13 ++ .../ExternalValuesMapperIntegrationIT.java | 16 -- .../mapper/AbstractGeometryFieldMapper.java | 137 ++++++------------ .../AbstractPointGeometryFieldMapper.java | 60 ++++---- .../AbstractShapeGeometryFieldMapper.java | 10 +- .../index/mapper/GeoPointFieldMapper.java | 97 ++++--------- .../index/mapper/GeoShapeFieldMapper.java | 28 ++-- .../index/mapper/GeoShapeIndexer.java | 17 +-- .../index/mapper/GeoShapeParser.java | 15 +- .../mapper/LegacyGeoShapeFieldMapper.java | 62 +++++--- .../index/mapper/LegacyGeoShapeIndexer.java | 61 -------- .../common/geo/GeometryIndexerTests.java | 44 +++--- .../index/mapper/DocumentParserTests.java | 24 +-- .../mapper/ExternalFieldMapperTests.java | 21 --- .../index/mapper/ExternalMapper.java | 55 +------ .../mapper/GeoPointFieldMapperTests.java | 17 ++- .../index/mapper/GeoPointFieldTypeTests.java | 8 +- .../search/geo/GeoShapeQueryTests.java | 2 +- .../xpack/spatial/SpatialUtils.java | 2 +- .../index/fielddata/GeoShapeValues.java | 4 +- .../GeoShapeWithDocValuesFieldMapper.java | 46 +++--- .../index/mapper/PointFieldMapper.java | 65 ++------- .../index/mapper/ShapeFieldMapper.java | 29 ++-- .../spatial/index/mapper/ShapeIndexer.java | 21 +-- .../index/fielddata/TriangleTreeTests.java | 2 +- .../mapper/CartesianFieldMapperTests.java | 2 +- .../LatLonShapeDocValuesQueryTests.java | 6 +- .../index/mapper/PointFieldTypeTests.java | 9 +- .../spatial/ingest/CircleProcessorTests.java | 6 +- .../xpack/spatial/util/GeoTestUtils.java | 4 +- 30 files changed, 318 insertions(+), 565 deletions(-) delete mode 100644 server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeIndexer.java diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/support/MapXContentParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/support/MapXContentParser.java index 1483a80d6e0b1..b87fe9c5c9b86 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/support/MapXContentParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/support/MapXContentParser.java @@ -11,12 +11,14 @@ import org.elasticsearch.common.xcontent.DeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentLocation; +import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import java.io.IOException; import java.math.BigDecimal; import java.math.BigInteger; import java.nio.CharBuffer; +import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Map; @@ -30,6 +32,17 @@ public class MapXContentParser extends AbstractXContentParser { private TokenIterator iterator; private boolean closed; + public static XContentParser wrapObject(Object sourceMap) throws IOException { + XContentParser parser = new MapXContentParser( + NamedXContentRegistry.EMPTY, + DeprecationHandler.IGNORE_DEPRECATIONS, + Collections.singletonMap("dummy_field", sourceMap), XContentType.JSON); + parser.nextToken(); // start object + parser.nextToken(); // field name + parser.nextToken(); // field value + return parser; + } + public MapXContentParser(NamedXContentRegistry xContentRegistry, DeprecationHandler deprecationHandler, Map map, XContentType xContentType) { super(xContentRegistry, deprecationHandler); diff --git a/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/ExternalValuesMapperIntegrationIT.java b/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/ExternalValuesMapperIntegrationIT.java index 00587af29fa42..a8c580d5657ea 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/ExternalValuesMapperIntegrationIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/index/mapper/ExternalValuesMapperIntegrationIT.java @@ -9,14 +9,11 @@ package org.elasticsearch.index.mapper; import org.elasticsearch.action.search.SearchResponse; -import org.elasticsearch.common.geo.ShapeRelation; -import org.elasticsearch.common.geo.builders.EnvelopeBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.search.fetch.subphase.highlight.HighlightBuilder; import org.elasticsearch.test.ESIntegTestCase; -import org.locationtech.jts.geom.Coordinate; import java.util.Collection; import java.util.Collections; @@ -99,19 +96,6 @@ public void testExternalValues() throws Exception { assertThat(response.getHits().getTotalHits().value, equalTo((long) 1)); - response = client().prepareSearch("test-idx") - .setPostFilter(QueryBuilders.geoDistanceQuery("field.point").point(42.0, 51.0).distance("1km")) - .execute().actionGet(); - - assertThat(response.getHits().getTotalHits().value, equalTo((long) 1)); - - response = client().prepareSearch("test-idx") - .setPostFilter(QueryBuilders.geoShapeQuery("field.shape", - new EnvelopeBuilder(new Coordinate(-101, 46), new Coordinate(-99, 44))).relation(ShapeRelation.WITHIN)) - .execute().actionGet(); - - assertThat(response.getHits().getTotalHits().value, equalTo((long) 1)); - response = client().prepareSearch("test-idx") .setPostFilter(QueryBuilders.termQuery("field.field", "foo")) .execute().actionGet(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/AbstractGeometryFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/AbstractGeometryFieldMapper.java index 1f2015eec88b3..d30db2e06f97f 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/AbstractGeometryFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/AbstractGeometryFieldMapper.java @@ -7,31 +7,29 @@ */ package org.elasticsearch.index.mapper; -import org.apache.lucene.index.IndexableField; import org.apache.lucene.search.Query; +import org.apache.lucene.util.SetOnce; +import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.Explicit; import org.elasticsearch.common.geo.GeoJsonGeometryFormat; -import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; -import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.support.MapXContentParser; import org.elasticsearch.index.analysis.NamedAnalyzer; import org.elasticsearch.index.query.SearchExecutionContext; import java.io.IOException; import java.io.UncheckedIOException; -import java.text.ParseException; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.function.Consumer; import java.util.function.Function; /** * Base field mapper class for all spatial field types */ -public abstract class AbstractGeometryFieldMapper extends FieldMapper { +public abstract class AbstractGeometryFieldMapper extends FieldMapper { public static Parameter> ignoreMalformedParam(Function> initializer, boolean ignoreMalformedByDefault) { @@ -42,24 +40,18 @@ public static Parameter> ignoreZValueParam(Function { - Processed prepareForIndexing(Parsed geometry); - Class processedClass(); - List indexShape(ParseContext context, Processed shape); - } - /** * Interface representing parser in geometry indexing pipeline. */ - public abstract static class Parser { + public abstract static class Parser { /** - * Parse the given xContent value to an object of type {@link Parsed}. The value can be + * Parse the given xContent value to one or more objects of type {@link T}. The value can be * in any supported format. */ - public abstract Parsed parse(XContentParser parser) throws IOException, ParseException; + public abstract void parse( + XContentParser parser, + CheckedConsumer consumer, + Consumer onMalformed) throws IOException; /** * Given a parsed value and a format string, formats the value into a plain Java object. @@ -67,27 +59,14 @@ public abstract static class Parser { * Supported formats include 'geojson' and 'wkt'. The different formats are defined * as subclasses of {@link org.elasticsearch.common.geo.GeometryFormat}. */ - public abstract Object format(Parsed value, String format); + public abstract Object format(T value, String format); - /** - * Parses the given value, then formats it according to the 'format' string. - * - * Used by value fetchers to validate and format geo objects - */ - public Object parseAndFormatObject(Object value, String format) { - Parsed geometry; - try (XContentParser parser = new MapXContentParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, - Collections.singletonMap("dummy_field", value), XContentType.JSON)) { - parser.nextToken(); // start object - parser.nextToken(); // field name - parser.nextToken(); // field value - geometry = parse(parser); + private void fetchFromSource(Object sourceMap, Consumer consumer, String format) { + try (XContentParser parser = MapXContentParser.wrapObject(sourceMap)) { + parse(parser, v -> consumer.accept(format(v, format)), e -> {}); /* ignore malformed */ } catch (IOException e) { throw new UncheckedIOException(e); - } catch (ParseException e) { - throw new RuntimeException(e); } - return format(geometry, format); } } @@ -113,19 +92,22 @@ public final Query termQuery(Object value, SearchExecutionContext context) { public final ValueFetcher valueFetcher(SearchExecutionContext context, String format) { String geoFormat = format != null ? format : GeoJsonGeometryFormat.NAME; - Function valueParser = value -> geometryParser.parseAndFormatObject(value, geoFormat); if (parsesArrayValue) { return new ArraySourceValueFetcher(name(), context) { @Override protected Object parseSourceValue(Object value) { - return valueParser.apply(value); + List values = new ArrayList<>(); + geometryParser.fetchFromSource(value, values::add, geoFormat); + return values; } }; } else { return new SourceValueFetcher(name(), context) { @Override protected Object parseSourceValue(Object value) { - return valueParser.apply(value); + SetOnce holder = new SetOnce<>(); + geometryParser.fetchFromSource(value, holder::set, geoFormat); + return holder.get(); } }; } @@ -134,26 +116,24 @@ protected Object parseSourceValue(Object value) { private final Explicit ignoreMalformed; private final Explicit ignoreZValue; - private final Indexer indexer; - private final Parser parser; + private final Parser parser; protected AbstractGeometryFieldMapper(String simpleName, MappedFieldType mappedFieldType, Map indexAnalyzers, Explicit ignoreMalformed, Explicit ignoreZValue, MultiFields multiFields, CopyTo copyTo, - Indexer indexer, Parser parser) { + Parser parser) { super(simpleName, mappedFieldType, indexAnalyzers, multiFields, copyTo, false, null); this.ignoreMalformed = ignoreMalformed; this.ignoreZValue = ignoreZValue; - this.indexer = indexer; this.parser = parser; } protected AbstractGeometryFieldMapper(String simpleName, MappedFieldType mappedFieldType, Explicit ignoreMalformed, Explicit ignoreZValue, MultiFields multiFields, CopyTo copyTo, - Indexer indexer, Parser parser) { - this(simpleName, mappedFieldType, Collections.emptyMap(), ignoreMalformed, ignoreZValue, multiFields, copyTo, indexer, parser); + Parser parser) { + this(simpleName, mappedFieldType, Collections.emptyMap(), ignoreMalformed, ignoreZValue, multiFields, copyTo, parser); } @Override @@ -166,60 +146,25 @@ protected void parseCreateField(ParseContext context) throws IOException { throw new UnsupportedOperationException("Parsing is implemented in parse(), this method should NEVER be called"); } - protected abstract void addStoredFields(ParseContext context, Processed geometry); - protected abstract void addDocValuesFields(String name, Processed geometry, List fields, ParseContext context); - protected abstract void addMultiFields(ParseContext context, Processed geometry) throws IOException; + /** + * Build an index document using a parsed geometry + * @param context the ParseContext holding the document + * @param geometry the parsed geometry object + */ + protected abstract void index(ParseContext context, T geometry) throws IOException; - /** parsing logic for geometry indexing */ @Override - public void parse(ParseContext context) throws IOException { - MappedFieldType mappedFieldType = fieldType(); - - try { - Processed shape = context.parseExternalValue(indexer.processedClass()); - if (shape == null) { - Parsed geometry = parser.parse(context.parser()); - if (geometry == null) { - return; - } - shape = indexer.prepareForIndexing(geometry); - } - - List fields = new ArrayList<>(); - if (mappedFieldType.isSearchable() || mappedFieldType.hasDocValues()) { - fields.addAll(indexer.indexShape(context, shape)); - } - - // indexed: - List indexedFields = new ArrayList<>(); - if (mappedFieldType.isSearchable()) { - indexedFields.addAll(fields); - } - // stored: - if (fieldType().isStored()) { - addStoredFields(context, shape); - } - // docValues: - if (fieldType().hasDocValues()) { - addDocValuesFields(mappedFieldType.name(), shape, fields, context); - } else if (fieldType().isStored() || fieldType().isSearchable()) { - createFieldNamesField(context); - } - - // add the indexed fields to the doc: - for (IndexableField field : indexedFields) { - context.doc().add(field); - } - - // add multifields (e.g., used for completion suggester) - addMultiFields(context, shape); - } catch (Exception e) { - if (ignoreMalformed.value() == false) { - throw new MapperParsingException("failed to parse field [{}] of type [{}]", e, fieldType().name(), - fieldType().typeName()); - } - context.addIgnoredField(mappedFieldType.name()); - } + public final void parse(ParseContext context) throws IOException { + parser.parse(context.parser(), v -> index(context, v), e -> { + if (ignoreMalformed()) { + context.addIgnoredField(fieldType().name()); + } else { + throw new MapperParsingException( + "Failed to parse field [" + fieldType().name() + "] of type [" + contentType() + "]", + e + ); + } + }); } public boolean ignoreMalformed() { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/AbstractPointGeometryFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/AbstractPointGeometryFieldMapper.java index 5259f5992aaeb..8fc2f8752bc89 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/AbstractPointGeometryFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/AbstractPointGeometryFieldMapper.java @@ -9,6 +9,7 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.CheckedBiFunction; +import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.Explicit; import org.elasticsearch.common.TriFunction; import org.elasticsearch.common.geo.GeoPoint; @@ -20,15 +21,12 @@ import org.elasticsearch.index.mapper.Mapper.TypeParser.ParserContext; import java.io.IOException; -import java.text.ParseException; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; +import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Supplier; /** Base class for for spatial fields that only support indexing points */ -public abstract class AbstractPointGeometryFieldMapper extends AbstractGeometryFieldMapper { +public abstract class AbstractPointGeometryFieldMapper extends AbstractGeometryFieldMapper { public static Parameter nullValueParam(Function initializer, TriFunction parser, @@ -41,8 +39,8 @@ public static Parameter nullValueParam(Function ignoreMalformed, Explicit ignoreZValue, ParsedPoint nullValue, CopyTo copyTo, - Indexer indexer, Parser parser) { - super(simpleName, mappedFieldType, ignoreMalformed, ignoreZValue, multiFields, copyTo, indexer, parser); + Parser parser) { + super(simpleName, mappedFieldType, ignoreMalformed, ignoreZValue, multiFields, copyTo, parser); this.nullValue = nullValue; } @@ -67,7 +65,7 @@ default boolean isNormalizable(double coord) { } /** A parser implementation that can parse the various point formats */ - public static class PointParser

extends Parser> { + public static class PointParser

extends Parser

{ /** * Note that this parser is only used for formatting values. */ @@ -88,7 +86,7 @@ public PointParser(String field, this.field = field; this.pointSupplier = pointSupplier; this.objectParser = objectParser; - this.nullValue = nullValue; + this.nullValue = nullValue == null ? null : process(nullValue); this.ignoreZValue = ignoreZValue; this.ignoreMalformed = ignoreMalformed; this.geometryParser = new GeometryParser(true, true, true); @@ -104,12 +102,14 @@ private P process(P in) { } @Override - public List

parse(XContentParser parser) throws IOException, ParseException { - + public void parse( + XContentParser parser, + CheckedConsumer consumer, + Consumer onMalformed + ) throws IOException { if (parser.currentToken() == XContentParser.Token.START_ARRAY) { XContentParser.Token token = parser.nextToken(); P point = pointSupplier.get(); - ArrayList

points = new ArrayList<>(); if (token == XContentParser.Token.VALUE_NUMBER) { double x = parser.doubleValue(); parser.nextToken(); @@ -122,35 +122,41 @@ public List

parse(XContentParser parser) throws IOException, ParseException { } point.resetCoords(x, y); - points.add(process(point)); + consumer.accept(process(point)); } else { while (token != XContentParser.Token.END_ARRAY) { - points.add(process(objectParser.apply(parser, point))); + parseAndConsumeFromObject(parser, point, consumer, onMalformed); point = pointSupplier.get(); token = parser.nextToken(); } } - return points; } else if (parser.currentToken() == XContentParser.Token.VALUE_NULL) { - if (nullValue == null) { - return null; - } else { - return Collections.singletonList(nullValue); + if (nullValue != null) { + consumer.accept(nullValue); } } else { - return Collections.singletonList(process(objectParser.apply(parser, pointSupplier.get()))); + parseAndConsumeFromObject(parser, pointSupplier.get(), consumer, onMalformed); + } + } + + private void parseAndConsumeFromObject( + XContentParser parser, + P point, + CheckedConsumer consumer, + Consumer onMalformed + ) { + try { + point = objectParser.apply(parser, point); + consumer.accept(process(point)); + } catch (Exception e) { + onMalformed.accept(e); } } @Override - public Object format(List

points, String format) { - List result = new ArrayList<>(); + public Object format(P point, String format) { GeometryFormat geometryFormat = geometryParser.geometryFormat(format); - for (ParsedPoint point : points) { - Geometry geometry = point.asGeometry(); - result.add(geometryFormat.toXContentAsObject(geometry)); - } - return result; + return geometryFormat.toXContentAsObject(point.asGeometry()); } } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/AbstractShapeGeometryFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/AbstractShapeGeometryFieldMapper.java index 1413e3df7564d..57dfb6189ccf3 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/AbstractShapeGeometryFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/AbstractShapeGeometryFieldMapper.java @@ -19,7 +19,7 @@ /** * Base class for {@link GeoShapeFieldMapper} and {@link LegacyGeoShapeFieldMapper} */ -public abstract class AbstractShapeGeometryFieldMapper extends AbstractGeometryFieldMapper { +public abstract class AbstractShapeGeometryFieldMapper extends AbstractGeometryFieldMapper { public static Parameter> coerceParam(Function> initializer, boolean coerceByDefault) { @@ -56,8 +56,8 @@ protected AbstractShapeGeometryFieldMapper(String simpleName, MappedFieldType ma Explicit ignoreMalformed, Explicit coerce, Explicit ignoreZValue, Explicit orientation, MultiFields multiFields, CopyTo copyTo, - Indexer indexer, Parser parser) { - super(simpleName, mappedFieldType, indexAnalyzers, ignoreMalformed, ignoreZValue, multiFields, copyTo, indexer, parser); + Parser parser) { + super(simpleName, mappedFieldType, indexAnalyzers, ignoreMalformed, ignoreZValue, multiFields, copyTo, parser); this.coerce = coerce; this.orientation = orientation; } @@ -66,9 +66,9 @@ protected AbstractShapeGeometryFieldMapper(String simpleName, MappedFieldType ma Explicit ignoreMalformed, Explicit coerce, Explicit ignoreZValue, Explicit orientation, MultiFields multiFields, CopyTo copyTo, - Indexer indexer, Parser parser) { + Parser parser) { this(simpleName, mappedFieldType, Collections.emptyMap(), - ignoreMalformed, coerce, ignoreZValue, orientation, multiFields, copyTo, indexer, parser); + ignoreMalformed, coerce, ignoreZValue, orientation, multiFields, copyTo, parser); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java index f8c9b84c0d8a0..0f17fbb77d522 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/GeoPointFieldMapper.java @@ -11,7 +11,6 @@ import org.apache.lucene.document.LatLonPoint; import org.apache.lucene.document.StoredField; import org.apache.lucene.geo.LatLonGeometry; -import org.apache.lucene.index.IndexableField; import org.apache.lucene.search.IndexOrDocValuesQuery; import org.apache.lucene.search.MatchNoDocsQuery; import org.apache.lucene.search.Query; @@ -32,7 +31,6 @@ import org.elasticsearch.search.lookup.SearchLookup; import java.io.IOException; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -44,7 +42,7 @@ * * Uses lucene 6 LatLonPoint encoding */ -public class GeoPointFieldMapper extends AbstractPointGeometryFieldMapper, List> { +public class GeoPointFieldMapper extends AbstractPointGeometryFieldMapper { public static final String CONTENT_TYPE = "geo_point"; @@ -102,14 +100,20 @@ private static ParsedGeoPoint parseNullValue(Object nullValue, boolean ignoreZVa @Override public FieldMapper build(ContentPath contentPath) { - Parser> geoParser = new PointParser<>(name, ParsedGeoPoint::new, (parser, point) -> { - GeoUtils.parseGeoPoint(parser, point, ignoreZValue.get().value()); - return point; - }, (ParsedGeoPoint) nullValue.get(), ignoreZValue.get().value(), ignoreMalformed.get().value()); + Parser geoParser = new PointParser<>( + name, + ParsedGeoPoint::new, + (parser, point) -> { + GeoUtils.parseGeoPoint(parser, point, ignoreZValue.get().value()); + return point; + }, + (ParsedGeoPoint) nullValue.get(), + ignoreZValue.get().value(), + ignoreMalformed.get().value()); GeoPointFieldType ft = new GeoPointFieldType(buildFullName(contentPath), indexed.get(), stored.get(), hasDocValues.get(), geoParser, meta.get()); return new GeoPointFieldMapper(name, ft, multiFieldsBuilder.build(this, contentPath), - copyTo.build(), new GeoPointIndexer(ft), geoParser, this); + copyTo.build(), geoParser, this); } } @@ -120,12 +124,11 @@ public FieldMapper build(ContentPath contentPath) { public GeoPointFieldMapper(String simpleName, MappedFieldType mappedFieldType, MultiFields multiFields, CopyTo copyTo, - Indexer, List> indexer, - Parser> parser, + Parser parser, Builder builder) { super(simpleName, mappedFieldType, multiFields, builder.ignoreMalformed.get(), builder.ignoreZValue.get(), builder.nullValue.get(), - copyTo, indexer, parser); + copyTo, parser); this.builder = builder; } @@ -135,39 +138,20 @@ public FieldMapper.Builder getMergeBuilder() { } @Override - protected void addStoredFields(ParseContext context, List points) { - for (GeoPoint point : points) { - context.doc().add(new StoredField(fieldType().name(), point.toString())); + protected void index(ParseContext context, ParsedGeoPoint geometry) throws IOException { + if (fieldType().isSearchable()) { + context.doc().add(new LatLonPoint(fieldType().name(), geometry.lat(), geometry.lon())); } - } - - @Override - protected void addMultiFields(ParseContext context, List points) throws IOException { - // @todo phase out geohash (which is currently used in the CompletionSuggester) - if (points.isEmpty()) { - return; + if (fieldType().hasDocValues()) { + context.doc().add(new LatLonDocValuesField(fieldType().name(), geometry.lat(), geometry.lon())); + } else if (fieldType().isStored() || fieldType().isSearchable()) { + createFieldNamesField(context); } - - StringBuilder s = new StringBuilder(); - if (points.size() > 1) { - s.append('['); - } - s.append(points.get(0).geohash()); - for (int i = 1; i < points.size(); ++i) { - s.append(','); - s.append(points.get(i).geohash()); - } - if (points.size() > 1) { - s.append(']'); - } - multiFields.parse(this, context.createExternalValueContext(s)); - } - - @Override - protected void addDocValuesFields(String name, List points, List fields, ParseContext context) { - for (GeoPoint point : points) { - context.doc().add(new LatLonDocValuesField(fieldType().name(), point.lat(), point.lon())); + if (fieldType().isStored()) { + context.doc().add(new StoredField(fieldType().name(), geometry.toString())); } + // TODO phase out geohash (which is currently used in the CompletionSuggester) + multiFields.parse(this, context.createExternalValueContext(geometry.geohash())); } @Override @@ -286,35 +270,4 @@ public int hashCode() { return super.hashCode(); } } - - protected static class GeoPointIndexer implements Indexer, List> { - - protected final GeoPointFieldType fieldType; - - GeoPointIndexer(GeoPointFieldType fieldType) { - this.fieldType = fieldType; - } - - @Override - public List prepareForIndexing(List geoPoints) { - if (geoPoints == null || geoPoints.isEmpty()) { - return Collections.emptyList(); - } - return geoPoints; - } - - @Override - public Class> processedClass() { - return (Class>)(Object)List.class; - } - - @Override - public List indexShape(ParseContext context, List points) { - ArrayList fields = new ArrayList<>(points.size()); - for (GeoPoint point : points) { - fields.add(new LatLonPoint(fieldType.name(), point.lat(), point.lon())); - } - return fields; - } - } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeFieldMapper.java index 6fe04515b49f0..2069a5e3878a3 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeFieldMapper.java @@ -9,7 +9,6 @@ import org.apache.lucene.document.LatLonShape; import org.apache.lucene.geo.LatLonGeometry; -import org.apache.lucene.index.IndexableField; import org.apache.lucene.search.MatchNoDocsQuery; import org.apache.lucene.search.Query; import org.elasticsearch.Version; @@ -19,9 +18,10 @@ import org.elasticsearch.common.geo.ShapeRelation; import org.elasticsearch.common.geo.builders.ShapeBuilder.Orientation; import org.elasticsearch.geometry.Geometry; -import org.elasticsearch.index.query.SearchExecutionContext; import org.elasticsearch.index.query.QueryShardException; +import org.elasticsearch.index.query.SearchExecutionContext; +import java.io.IOException; import java.util.Arrays; import java.util.List; import java.util.Map; @@ -46,7 +46,7 @@ *

* "field" : "POLYGON ((100.0 0.0, 101.0 0.0, 101.0 1.0, 100.0 1.0, 100.0 0.0)) */ -public class GeoShapeFieldMapper extends AbstractShapeGeometryFieldMapper { +public class GeoShapeFieldMapper extends AbstractShapeGeometryFieldMapper { public static final String CONTENT_TYPE = "geo_shape"; @@ -147,10 +147,12 @@ public Query geoShapeQuery(Geometry shape, String fieldName, ShapeRelation relat }; private final Builder builder; + private final GeoShapeIndexer indexer; public GeoShapeFieldMapper(String simpleName, MappedFieldType mappedFieldType, MultiFields multiFields, CopyTo copyTo, - Indexer indexer, Parser parser, Builder builder) { + GeoShapeIndexer indexer, + Parser parser, Builder builder) { super(simpleName, mappedFieldType, builder.ignoreMalformed.get(), @@ -159,9 +161,9 @@ public GeoShapeFieldMapper(String simpleName, MappedFieldType mappedFieldType, builder.orientation.get(), multiFields, copyTo, - indexer, parser); this.builder = builder; + this.indexer = indexer; } @Override @@ -185,19 +187,9 @@ protected void checkIncomingMergeType(FieldMapper mergeWith) { } @Override - protected void addStoredFields(ParseContext context, Geometry geometry) { - // noop: we currently do not store geo_shapes - // @todo store as geojson string? - } - - @Override - protected void addDocValuesFields(String name, Geometry geometry, List fields, ParseContext context) { - // we will throw a mapping exception before we get here - } - - @Override - protected void addMultiFields(ParseContext context, Geometry geometry) { - // noop (completion suggester currently not compatible with geo_shape) + protected void index(ParseContext context, Geometry geometry) throws IOException { + context.doc().addAll(indexer.indexShape(geometry)); + createFieldNamesField(context); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeIndexer.java b/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeIndexer.java index e005d7af55d61..992c8d56b1b78 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeIndexer.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeIndexer.java @@ -14,8 +14,8 @@ import org.apache.lucene.index.IndexableField; import org.elasticsearch.common.geo.GeoLineDecomposer; import org.elasticsearch.common.geo.GeoPolygonDecomposer; -import org.elasticsearch.common.geo.GeoShapeUtils; import org.elasticsearch.common.geo.GeoShapeType; +import org.elasticsearch.common.geo.GeoShapeUtils; import org.elasticsearch.common.geo.GeoUtils; import org.elasticsearch.geometry.Circle; import org.elasticsearch.geometry.Geometry; @@ -32,6 +32,7 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import static org.elasticsearch.common.geo.GeoUtils.normalizePoint; @@ -39,7 +40,7 @@ /** * Utility class that converts geometries into Lucene-compatible form for indexing in a geo_shape field. */ -public class GeoShapeIndexer implements AbstractGeometryFieldMapper.Indexer { +public class GeoShapeIndexer { private final boolean orientation; private final String name; @@ -166,14 +167,12 @@ public Geometry visit(Rectangle rectangle) { }); } - @Override - public Class processedClass() { - return Geometry.class; - } - - @Override - public List indexShape(ParseContext context, Geometry shape) { + public List indexShape(Geometry shape) { LuceneGeometryIndexer visitor = new LuceneGeometryIndexer(name); + shape = prepareForIndexing(shape); + if (shape == null) { + return Collections.emptyList(); + } shape.visit(visitor); return visitor.fields(); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeParser.java b/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeParser.java index ea923b70c4b03..0ee9137fccbe0 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/GeoShapeParser.java @@ -8,12 +8,15 @@ package org.elasticsearch.index.mapper; +import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.geo.GeometryParser; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.geometry.Geometry; import java.io.IOException; import java.text.ParseException; +import java.util.function.Consumer; public class GeoShapeParser extends AbstractGeometryFieldMapper.Parser { private final GeometryParser geometryParser; @@ -23,8 +26,16 @@ public GeoShapeParser(GeometryParser geometryParser) { } @Override - public Geometry parse(XContentParser parser) throws IOException, ParseException { - return geometryParser.parse(parser); + public void parse( + XContentParser parser, + CheckedConsumer consumer, + Consumer onMalformed + ) throws IOException { + try { + consumer.accept(geometryParser.parse(parser)); + } catch (ParseException | ElasticsearchParseException | IllegalArgumentException e) { + onMalformed.accept(e); + } } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapper.java index f5d2c4245b068..0b8cd6fd80532 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeFieldMapper.java @@ -7,7 +7,6 @@ */ package org.elasticsearch.index.mapper; -import org.apache.lucene.index.IndexableField; import org.apache.lucene.search.Query; import org.apache.lucene.spatial.prefix.PrefixTreeStrategy; import org.apache.lucene.spatial.prefix.RecursivePrefixTreeStrategy; @@ -18,12 +17,14 @@ import org.apache.lucene.spatial.prefix.tree.SpatialPrefixTree; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.Version; +import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.Explicit; import org.elasticsearch.common.geo.GeoUtils; import org.elasticsearch.common.geo.GeometryParser; import org.elasticsearch.common.geo.ShapeRelation; import org.elasticsearch.common.geo.ShapesAvailability; import org.elasticsearch.common.geo.SpatialStrategy; +import org.elasticsearch.common.geo.XShapeCollection; import org.elasticsearch.common.geo.builders.ShapeBuilder; import org.elasticsearch.common.geo.builders.ShapeBuilder.Orientation; import org.elasticsearch.common.geo.parsers.ShapeParser; @@ -35,10 +36,11 @@ import org.elasticsearch.geometry.Geometry; import org.elasticsearch.index.query.LegacyGeoShapeQueryProcessor; import org.elasticsearch.index.query.SearchExecutionContext; +import org.locationtech.spatial4j.shape.Point; import org.locationtech.spatial4j.shape.Shape; +import org.locationtech.spatial4j.shape.jts.JtsGeometry; import java.io.IOException; -import java.text.ParseException; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; @@ -46,6 +48,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.function.Consumer; /** * FieldMapper for indexing {@link org.locationtech.spatial4j.shape.Shape}s. @@ -70,7 +73,7 @@ * @deprecated use {@link GeoShapeFieldMapper} */ @Deprecated -public class LegacyGeoShapeFieldMapper extends AbstractShapeGeometryFieldMapper, Shape> { +public class LegacyGeoShapeFieldMapper extends AbstractShapeGeometryFieldMapper> { public static final String CONTENT_TYPE = "geo_shape"; @@ -292,14 +295,12 @@ public LegacyGeoShapeFieldMapper build(ContentPath contentPath) { GeoShapeFieldType ft = buildFieldType(parser, contentPath); return new LegacyGeoShapeFieldMapper(name, ft, multiFieldsBuilder.build(this, contentPath), copyTo.build(), - new LegacyGeoShapeIndexer(ft), parser, this); + parser, this); } } private static class LegacyGeoShapeParser extends Parser> { - /** - * Note that this parser is only used for formatting values. - */ + private final GeometryParser geometryParser; private LegacyGeoShapeParser() { @@ -307,8 +308,16 @@ private LegacyGeoShapeParser() { } @Override - public ShapeBuilder parse(XContentParser parser) throws IOException, ParseException { - return ShapeParser.parse(parser); + public void parse( + XContentParser parser, + CheckedConsumer, IOException> consumer, + Consumer onMalformed + ) throws IOException { + try { + consumer.accept(ShapeParser.parse(parser)); + } catch (ElasticsearchParseException e) { + onMalformed.accept(e); + } } @Override @@ -439,11 +448,11 @@ public PrefixTreeStrategy resolvePrefixTreeStrategy(String strategyName) { public LegacyGeoShapeFieldMapper(String simpleName, MappedFieldType mappedFieldType, MultiFields multiFields, CopyTo copyTo, - LegacyGeoShapeIndexer indexer, LegacyGeoShapeParser parser, + LegacyGeoShapeParser parser, Builder builder) { super(simpleName, mappedFieldType, Collections.singletonMap(mappedFieldType.name(), Lucene.KEYWORD_ANALYZER), builder.ignoreMalformed.get(), builder.coerce.get(), builder.ignoreZValue.get(), builder.orientation.get(), - multiFields, copyTo, indexer, parser); + multiFields, copyTo, parser); this.indexCreatedVersion = builder.indexCreatedVersion; this.builder = builder; } @@ -458,18 +467,25 @@ String strategy() { } @Override - protected void addStoredFields(ParseContext context, Shape geometry) { - // noop: we do not store geo_shapes; and will not store legacy geo_shape types - } - - @Override - protected void addDocValuesFields(String name, Shape geometry, List fields, ParseContext context) { - // doc values are not supported - } - - @Override - protected void addMultiFields(ParseContext context, Shape geometry) { - // noop (completion suggester currently not compatible with geo_shape) + protected void index(ParseContext context, ShapeBuilder shapeBuilder) throws IOException { + Shape shape = shapeBuilder.buildS4J(); + if (fieldType().pointsOnly()) { + // index configured for pointsOnly + if (shape instanceof XShapeCollection && ((XShapeCollection) shape).pointsOnly()) { + // MULTIPOINT data: index each point separately + @SuppressWarnings("unchecked") List shapes = ((XShapeCollection) shape).getShapes(); + for (Shape s : shapes) { + context.doc().addAll(Arrays.asList(fieldType().defaultPrefixTreeStrategy().createIndexableFields(s))); + } + return; + } else if (shape instanceof Point == false) { + throw new MapperParsingException("[{" + fieldType().name() + "}] is configured for points only but a " + + ((shape instanceof JtsGeometry) ? ((JtsGeometry)shape).getGeom().getGeometryType() : shape.getClass()) + + " was found"); + } + } + context.doc().addAll(Arrays.asList(fieldType().defaultPrefixTreeStrategy().createIndexableFields(shape))); + createFieldNamesField(context); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeIndexer.java b/server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeIndexer.java deleted file mode 100644 index d89b5dbb499dd..0000000000000 --- a/server/src/main/java/org/elasticsearch/index/mapper/LegacyGeoShapeIndexer.java +++ /dev/null @@ -1,61 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0 and the Server Side Public License, v 1; you may not use this file except - * in compliance with, at your election, the Elastic License 2.0 or the Server - * Side Public License, v 1. - */ - -package org.elasticsearch.index.mapper; - -import org.apache.lucene.index.IndexableField; -import org.elasticsearch.common.geo.XShapeCollection; -import org.elasticsearch.common.geo.builders.ShapeBuilder; -import org.locationtech.spatial4j.shape.Point; -import org.locationtech.spatial4j.shape.Shape; -import org.locationtech.spatial4j.shape.jts.JtsGeometry; - -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; - -public class LegacyGeoShapeIndexer implements AbstractGeometryFieldMapper.Indexer, Shape> { - - private LegacyGeoShapeFieldMapper.GeoShapeFieldType fieldType; - - public LegacyGeoShapeIndexer(LegacyGeoShapeFieldMapper.GeoShapeFieldType fieldType) { - this.fieldType = fieldType; - } - - - @Override - public Shape prepareForIndexing(ShapeBuilder shapeBuilder) { - return shapeBuilder.buildS4J(); - } - - @Override - public Class processedClass() { - return Shape.class; - } - - @Override - public List indexShape(ParseContext context, Shape shape) { - if (fieldType.pointsOnly()) { - // index configured for pointsOnly - if (shape instanceof XShapeCollection && XShapeCollection.class.cast(shape).pointsOnly()) { - // MULTIPOINT data: index each point separately - @SuppressWarnings("unchecked") List shapes = ((XShapeCollection) shape).getShapes(); - List fields = new ArrayList<>(); - for (Shape s : shapes) { - fields.addAll(Arrays.asList(fieldType.defaultPrefixTreeStrategy().createIndexableFields(s))); - } - return fields; - } else if (shape instanceof Point == false) { - throw new MapperParsingException("[{" + fieldType.name() + "}] is configured for points only but a " - + ((shape instanceof JtsGeometry) ? ((JtsGeometry)shape).getGeom().getGeometryType() : shape.getClass()) - + " was found"); - } - } - return Arrays.asList(fieldType.defaultPrefixTreeStrategy().createIndexableFields(shape)); - } -} diff --git a/server/src/test/java/org/elasticsearch/common/geo/GeometryIndexerTests.java b/server/src/test/java/org/elasticsearch/common/geo/GeometryIndexerTests.java index 15a32f8cec0c7..440460e209bba 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/GeometryIndexerTests.java +++ b/server/src/test/java/org/elasticsearch/common/geo/GeometryIndexerTests.java @@ -8,17 +8,6 @@ package org.elasticsearch.common.geo; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.instanceOf; -import static org.hamcrest.Matchers.not; - -import java.io.IOException; -import java.text.ParseException; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; -import java.util.List; - import org.apache.lucene.index.IndexableField; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; @@ -39,6 +28,17 @@ import org.elasticsearch.test.ESTestCase; import org.locationtech.spatial4j.exception.InvalidShapeException; +import java.io.IOException; +import java.text.ParseException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.not; + public class GeometryIndexerTests extends ESTestCase { GeoShapeIndexer indexer = new GeoShapeIndexer(true, "test"); @@ -293,7 +293,7 @@ public void testRectangle() { assertEquals(indexed, processed); // a rectangle is broken into two triangles - List fields = indexer.indexShape(null, indexed); + List fields = indexer.indexShape(indexed); assertEquals(fields.size(), 2); indexed = new Rectangle(179, -179, 10, -10); @@ -301,7 +301,7 @@ public void testRectangle() { assertEquals(indexed, processed); // a rectangle crossing the dateline is broken into 4 triangles - fields = indexer.indexShape(null, indexed); + fields = indexer.indexShape(indexed); assertEquals(fields.size(), 4); } @@ -311,7 +311,7 @@ public void testDegeneratedRectangles() { assertEquals(indexed, processed); // Rectangle is a line - List fields = indexer.indexShape(null, indexed); + List fields = indexer.indexShape(indexed); assertEquals(fields.size(), 1); indexed = new Rectangle(-179, -178, 10, 10); @@ -319,7 +319,7 @@ public void testDegeneratedRectangles() { assertEquals(indexed, processed); // Rectangle is a line - fields = indexer.indexShape(null, indexed); + fields = indexer.indexShape(indexed); assertEquals(fields.size(), 1); indexed = new Rectangle(-179, -179, 10, 10); @@ -327,7 +327,7 @@ public void testDegeneratedRectangles() { assertEquals(indexed, processed); // Rectangle is a point - fields = indexer.indexShape(null, indexed); + fields = indexer.indexShape(indexed); assertEquals(fields.size(), 1); indexed = new Rectangle(180, -179, 10, -10); @@ -335,7 +335,7 @@ public void testDegeneratedRectangles() { assertEquals(indexed, processed); // Rectangle crossing the dateline, one side is a line - fields = indexer.indexShape(null, indexed); + fields = indexer.indexShape(indexed); assertEquals(fields.size(), 3); indexed = new Rectangle(180, -179, 10, 10); @@ -344,7 +344,7 @@ public void testDegeneratedRectangles() { // Rectangle crossing the dateline, one side is a point, // other side a line - fields = indexer.indexShape(null, indexed); + fields = indexer.indexShape(indexed); assertEquals(fields.size(), 2); indexed = new Rectangle(-178, -180, 10, -10); @@ -352,7 +352,7 @@ public void testDegeneratedRectangles() { assertEquals(indexed, processed); // Rectangle crossing the dateline, one side is a line - fields = indexer.indexShape(null, indexed); + fields = indexer.indexShape(indexed); assertEquals(fields.size(), 3); indexed = new Rectangle(-178, -180, 10, 10); @@ -361,7 +361,7 @@ public void testDegeneratedRectangles() { // Rectangle crossing the dateline, one side is a point, // other side a line - fields = indexer.indexShape(null, indexed); + fields = indexer.indexShape(indexed); assertEquals(fields.size(), 2); indexed = new Rectangle(0.0, 1.0819389717881644E-299, 1.401298464324817E-45, 0.0); @@ -369,7 +369,7 @@ public void testDegeneratedRectangles() { assertEquals(indexed, processed); // Rectangle is a point - fields = indexer.indexShape(null, processed); + fields = indexer.indexShape(processed); assertEquals(fields.size(), 1); indexed = new Rectangle(-1.4017117476654298E-170, 0.0, 0.0, -2.415012082648633E-174); @@ -377,7 +377,7 @@ public void testDegeneratedRectangles() { assertEquals(indexed, processed); // Rectangle is a triangle but needs to be computed quantize - fields = indexer.indexShape(null, processed); + fields = indexer.indexShape(processed); assertEquals(fields.size(), 2); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java index 011b70ec49248..96b787afd0f98 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -1071,14 +1071,14 @@ public void testWithDynamicTemplates() throws Exception { Collections.singletonMap(field, "points"))); IndexableField[] fields = doc.rootDoc().getFields(field); assertThat(fields, arrayWithSize(2)); - assertThat(fields[0].fieldType(), sameInstance(LatLonDocValuesField.TYPE)); - assertThat(fields[1].fieldType(), sameInstance(LatLonPoint.TYPE)); + assertThat(fields[0].fieldType(), sameInstance(LatLonPoint.TYPE)); + assertThat(fields[1].fieldType(), sameInstance(LatLonDocValuesField.TYPE)); doc = mapper.parse(source("1", b -> b.field(field, new double[]{-71.34, 41.12}), null, Collections.singletonMap(field, "points"))); fields = doc.rootDoc().getFields(field); assertThat(fields, arrayWithSize(2)); - assertThat(fields[0].fieldType(), sameInstance(LatLonDocValuesField.TYPE)); - assertThat(fields[1].fieldType(), sameInstance(LatLonPoint.TYPE)); + assertThat(fields[0].fieldType(), sameInstance(LatLonPoint.TYPE)); + assertThat(fields[1].fieldType(), sameInstance(LatLonDocValuesField.TYPE)); doc = mapper.parse(source("1", b -> { b.startObject(field); @@ -1088,17 +1088,17 @@ public void testWithDynamicTemplates() throws Exception { }, null, Collections.singletonMap(field, "points"))); fields = doc.rootDoc().getFields(field); assertThat(fields, arrayWithSize(2)); - assertThat(fields[0].fieldType(), sameInstance(LatLonDocValuesField.TYPE)); - assertThat(fields[1].fieldType(), sameInstance(LatLonPoint.TYPE)); + assertThat(fields[0].fieldType(), sameInstance(LatLonPoint.TYPE)); + assertThat(fields[1].fieldType(), sameInstance(LatLonDocValuesField.TYPE)); doc = mapper.parse(source("1", b -> b.field(field, new String[]{"41.12,-71.34", "43,-72.34"}), null, Collections.singletonMap(field, "points"))); fields = doc.rootDoc().getFields(field); assertThat(fields, arrayWithSize(4)); - assertThat(fields[0].fieldType(), sameInstance(LatLonDocValuesField.TYPE)); + assertThat(fields[0].fieldType(), sameInstance(LatLonPoint.TYPE)); assertThat(fields[1].fieldType(), sameInstance(LatLonDocValuesField.TYPE)); assertThat(fields[2].fieldType(), sameInstance(LatLonPoint.TYPE)); - assertThat(fields[3].fieldType(), sameInstance(LatLonPoint.TYPE)); + assertThat(fields[3].fieldType(), sameInstance(LatLonDocValuesField.TYPE)); doc = mapper.parse(source("1", b -> { b.startArray(field); @@ -1115,10 +1115,10 @@ public void testWithDynamicTemplates() throws Exception { }, null, Collections.singletonMap(field, "points"))); fields = doc.rootDoc().getFields(field); assertThat(fields, arrayWithSize(4)); - assertThat(fields[0].fieldType(), sameInstance(LatLonDocValuesField.TYPE)); + assertThat(fields[0].fieldType(), sameInstance(LatLonPoint.TYPE)); assertThat(fields[1].fieldType(), sameInstance(LatLonDocValuesField.TYPE)); assertThat(fields[2].fieldType(), sameInstance(LatLonPoint.TYPE)); - assertThat(fields[3].fieldType(), sameInstance(LatLonPoint.TYPE)); + assertThat(fields[3].fieldType(), sameInstance(LatLonDocValuesField.TYPE)); doc = mapper.parse(source("1", b -> { b.startObject("address"); @@ -1127,8 +1127,8 @@ public void testWithDynamicTemplates() throws Exception { }, null, Collections.singletonMap("address.home", "points"))); fields = doc.rootDoc().getFields("address.home"); assertThat(fields, arrayWithSize(2)); - assertThat(fields[0].fieldType(), sameInstance(LatLonDocValuesField.TYPE)); - assertThat(fields[1].fieldType(), sameInstance(LatLonPoint.TYPE)); + assertThat(fields[0].fieldType(), sameInstance(LatLonPoint.TYPE)); + assertThat(fields[1].fieldType(), sameInstance(LatLonDocValuesField.TYPE)); } public void testDynamicTemplatesNotFound() throws Exception { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java index ab6928499108f..2e0a9b2932249 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ExternalFieldMapperTests.java @@ -10,13 +10,11 @@ import org.apache.lucene.index.IndexableField; import org.apache.lucene.util.BytesRef; -import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.plugins.Plugin; import java.util.Collection; import java.util.Collections; -import static org.hamcrest.Matchers.closeTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; @@ -36,13 +34,6 @@ public void testExternalValues() throws Exception { assertThat(doc.rootDoc().getField("field.bool"), notNullValue()); assertThat(doc.rootDoc().getField("field.bool").stringValue(), is("T")); - assertThat(doc.rootDoc().getField("field.point"), notNullValue()); - GeoPoint point = new GeoPoint().resetFromIndexableField(doc.rootDoc().getField("field.point")); - assertThat(point.lat(), closeTo(42.0, 1e-5)); - assertThat(point.lon(), closeTo(51.0, 1e-5)); - - assertThat(doc.rootDoc().getField("field.shape"), notNullValue()); - assertThat(doc.rootDoc().getField("field.field"), notNullValue()); assertThat(doc.rootDoc().getField("field.field").stringValue(), is("foo")); @@ -81,14 +72,6 @@ public void testExternalValuesWithMultifield() throws Exception { assertThat(doc.rootDoc().getField("field.bool"), notNullValue()); assertThat(doc.rootDoc().getField("field.bool").stringValue(), is("T")); - assertThat(doc.rootDoc().getField("field.point"), notNullValue()); - GeoPoint point = new GeoPoint().resetFromIndexableField(doc.rootDoc().getField("field.point")); - assertThat(point.lat(), closeTo(42.0, 1E-5)); - assertThat(point.lon(), closeTo(51.0, 1E-5)); - - IndexableField shape = doc.rootDoc().getField("field.shape"); - assertThat(shape, notNullValue()); - IndexableField field = doc.rootDoc().getField("field.text"); assertThat(field, notNullValue()); assertThat(field.stringValue(), is("foo")); @@ -132,10 +115,6 @@ public void testExternalValuesWithMultifieldTwoLevels() throws Exception { assertThat(doc.rootDoc().getField("field.bool"), notNullValue()); assertThat(doc.rootDoc().getField("field.bool").stringValue(), is("T")); - assertThat(doc.rootDoc().getField("field.point"), notNullValue()); - - assertThat(doc.rootDoc().getField("field.shape"), notNullValue()); - assertThat(doc.rootDoc().getField("field.text"), notNullValue()); assertThat(doc.rootDoc().getField("field.text").stringValue(), is("foo")); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ExternalMapper.java b/server/src/test/java/org/elasticsearch/index/mapper/ExternalMapper.java index 178b3945a6665..5d3a7c249bf82 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ExternalMapper.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ExternalMapper.java @@ -9,11 +9,7 @@ package org.elasticsearch.index.mapper; import org.apache.lucene.analysis.standard.StandardAnalyzer; -import org.elasticsearch.Version; import org.elasticsearch.common.collect.Iterators; -import org.elasticsearch.common.geo.GeoPoint; -import org.elasticsearch.common.geo.builders.PointBuilder; -import org.elasticsearch.geometry.Point; import org.elasticsearch.index.analysis.AnalyzerScope; import org.elasticsearch.index.analysis.IndexAnalyzers; import org.elasticsearch.index.analysis.NamedAnalyzer; @@ -22,7 +18,6 @@ import java.io.IOException; import java.nio.charset.Charset; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.Iterator; @@ -54,21 +49,15 @@ public static class Builder extends FieldMapper.Builder { private final BinaryFieldMapper.Builder binBuilder = new BinaryFieldMapper.Builder(Names.FIELD_BIN); private final BooleanFieldMapper.Builder boolBuilder = new BooleanFieldMapper.Builder(Names.FIELD_BOOL, ScriptCompiler.NONE); - private final GeoPointFieldMapper.Builder latLonPointBuilder = new GeoPointFieldMapper.Builder(Names.FIELD_POINT, false); - private final GeoShapeFieldMapper.Builder shapeBuilder = new GeoShapeFieldMapper.Builder(Names.FIELD_SHAPE, false, true); - private final LegacyGeoShapeFieldMapper.Builder legacyShapeBuilder - = new LegacyGeoShapeFieldMapper.Builder(Names.FIELD_SHAPE, Version.CURRENT, false, true); private final Mapper.Builder stringBuilder; private final String generatedValue; private final String mapperName; - private final Version indexCreatedVersion; - public Builder(String name, String generatedValue, String mapperName, Version indexCreatedVersion) { + public Builder(String name, String generatedValue, String mapperName) { super(name); this.stringBuilder = new TextFieldMapper.Builder(name, INDEX_ANALYZERS).store(false); this.generatedValue = generatedValue; this.mapperName = mapperName; - this.indexCreatedVersion = indexCreatedVersion; } @Override @@ -81,21 +70,16 @@ public ExternalMapper build(ContentPath contentPath) { contentPath.add(name); BinaryFieldMapper binMapper = binBuilder.build(contentPath); BooleanFieldMapper boolMapper = boolBuilder.build(contentPath); - GeoPointFieldMapper pointMapper = (GeoPointFieldMapper) latLonPointBuilder.build(contentPath); - AbstractShapeGeometryFieldMapper shapeMapper = (indexCreatedVersion.before(Version.V_6_6_0)) - ? legacyShapeBuilder.build(contentPath) - : shapeBuilder.build(contentPath); - FieldMapper stringMapper = (FieldMapper) stringBuilder.build(contentPath); + FieldMapper stringMapper = (FieldMapper)stringBuilder.build(contentPath); contentPath.remove(); return new ExternalMapper(name, buildFullName(contentPath), generatedValue, mapperName, binMapper, boolMapper, - pointMapper, shapeMapper, stringMapper, indexCreatedVersion, - multiFieldsBuilder.build(this, contentPath), copyTo.build()); + stringMapper, multiFieldsBuilder.build(this, contentPath), copyTo.build()); } } public static TypeParser parser(String mapperName, String generatedValue) { - return new TypeParser((n, c) -> new Builder(n, generatedValue, mapperName, c.indexVersionCreated())); + return new TypeParser((n, c) -> new Builder(n, generatedValue, mapperName)); } static class ExternalFieldType extends TermBasedFieldType { @@ -120,27 +104,19 @@ public ValueFetcher valueFetcher(SearchExecutionContext context, String format) private final BinaryFieldMapper binMapper; private final BooleanFieldMapper boolMapper; - private final GeoPointFieldMapper pointMapper; - private final AbstractShapeGeometryFieldMapper shapeMapper; private final FieldMapper stringMapper; - private final Version indexCreatedVersion; - public ExternalMapper(String simpleName, String contextName, String generatedValue, String mapperName, - BinaryFieldMapper binMapper, BooleanFieldMapper boolMapper, GeoPointFieldMapper pointMapper, - AbstractShapeGeometryFieldMapper shapeMapper, FieldMapper stringMapper, - Version indexCreatedVersion, + BinaryFieldMapper binMapper, BooleanFieldMapper boolMapper, + FieldMapper stringMapper, MultiFields multiFields, CopyTo copyTo) { super(simpleName, new ExternalFieldType(contextName, true, true, false), multiFields, copyTo); this.generatedValue = generatedValue; this.mapperName = mapperName; this.binMapper = binMapper; this.boolMapper = boolMapper; - this.pointMapper = pointMapper; - this.shapeMapper = shapeMapper; this.stringMapper = stringMapper; - this.indexCreatedVersion = indexCreatedVersion; } @Override @@ -150,21 +126,6 @@ public void parse(ParseContext context) throws IOException { boolMapper.parse(context.createExternalValueContext(true)); - // Let's add a Dummy Point - double lat = 42.0; - double lng = 51.0; - ArrayList points = new ArrayList<>(); - points.add(new GeoPoint(lat, lng)); - pointMapper.parse(context.createExternalValueContext(points)); - - // Let's add a Dummy Shape - if (shapeMapper instanceof GeoShapeFieldMapper) { - shapeMapper.parse(context.createExternalValueContext(new Point(-100, 45))); - } else { - PointBuilder pb = new PointBuilder(-100, 45); - shapeMapper.parse(context.createExternalValueContext(pb.buildS4J())); - } - context = context.createExternalValueContext(generatedValue); // Let's add a Original String @@ -180,12 +141,12 @@ protected void parseCreateField(ParseContext context) { @Override public Iterator iterator() { - return Iterators.concat(super.iterator(), Arrays.asList(binMapper, boolMapper, pointMapper, shapeMapper, stringMapper).iterator()); + return Iterators.concat(super.iterator(), Arrays.asList(binMapper, boolMapper, stringMapper).iterator()); } @Override public FieldMapper.Builder getMergeBuilder() { - return new Builder(simpleName(), generatedValue, mapperName, indexCreatedVersion); + return new Builder(simpleName(), generatedValue, mapperName); } @Override diff --git a/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java index 1933f958f5e54..0a0b0391de540 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java @@ -220,6 +220,21 @@ public void testMultiField() throws Exception { assertThat(doc.getField("field.latlon").stringValue(), equalTo("s093jd0k72s1")); } + public void testMultiFieldWithMultipleValues() throws Exception { + DocumentMapper mapper = createDocumentMapper(fieldMapping(b -> { + b.field("type", "geo_point").field("doc_values", false); + b.startObject("fields"); + { + b.startObject("geohash").field("type", "keyword").field("doc_values", false).endObject(); + } + b.endObject(); + })); + ParseContext.Document doc = mapper.parse(source(b -> b.array("field", "POINT (2 3)", "POINT (4 5)"))).rootDoc(); + assertThat(doc.getFields("field.geohash"), arrayWithSize(2)); + assertThat(doc.getFields("field.geohash")[0].binaryValue().utf8ToString(), equalTo("s093jd0k72s1")); + assertThat(doc.getFields("field.geohash")[1].binaryValue().utf8ToString(), equalTo("s0fu7n0xng81")); + } + public void testNullValue() throws Exception { DocumentMapper mapper = createDocumentMapper( fieldMapping(b -> b.field("type", "geo_point")) @@ -298,7 +313,7 @@ public void testInvalidGeohashNotIgnored() throws Exception { MapperParsingException.class, () -> mapper.parse(source(b -> b.field("field", "1234.333"))) ); - assertThat(e.getMessage(), containsString("failed to parse field [field] of type [geo_point]")); + assertThat(e.getMessage(), containsString("Failed to parse field [field] of type [geo_point]")); assertThat(e.getRootCause().getMessage(), containsString("unsupported symbol [.] in geohash [1234.333]")); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldTypeTests.java index ffca3218024f5..d96d69c516d53 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldTypeTests.java @@ -44,11 +44,9 @@ public void testFetchSourceValue() throws IOException { assertEquals(Arrays.asList(wktPoint, otherWktPoint), fetchSourceValue(mapper, sourceValue, "wkt")); // Test a list of points in [lat,lon] array format with one malformed - // TODO Point Field parsers have a weird `ignore_malformed` impl that still throws errors - // on many types of malformed input - // sourceValue = List.of(List.of(42,0, 27.1), List.of("a", "b"), List.of(30.0, 50.0)); - // assertEquals(List.of(jsonPoint, otherJsonPoint), fetchSourceValue(mapper, sourceValue, null)); - // assertEquals(List.of(wktPoint, otherWktPoint), fetchSourceValue(mapper, sourceValue, "wkt")); + sourceValue = Arrays.asList(Arrays.asList(42.0, 27.1), Arrays.asList("a", "b"), Arrays.asList(30.0, 50.0)); + assertEquals(Arrays.asList(jsonPoint, otherJsonPoint), fetchSourceValue(mapper, sourceValue, null)); + assertEquals(Arrays.asList(wktPoint, otherWktPoint), fetchSourceValue(mapper, sourceValue, "wkt")); // Test a single point in well-known text format. sourceValue = "POINT (42.0 27.1)"; diff --git a/server/src/test/java/org/elasticsearch/search/geo/GeoShapeQueryTests.java b/server/src/test/java/org/elasticsearch/search/geo/GeoShapeQueryTests.java index be0820a464a98..340b578d198d9 100644 --- a/server/src/test/java/org/elasticsearch/search/geo/GeoShapeQueryTests.java +++ b/server/src/test/java/org/elasticsearch/search/geo/GeoShapeQueryTests.java @@ -538,7 +538,7 @@ public void testPointsOnly() throws Exception { .setRefreshPolicy(IMMEDIATE).get(); } catch (MapperParsingException e) { // RandomShapeGenerator created something other than a POINT type, verify the correct exception is thrown - assertThat(e.getCause().getMessage(), containsString("is configured for points only")); + assertThat(e.getMessage(), containsString("is configured for points only")); return; } diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/SpatialUtils.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/SpatialUtils.java index e659f0cacfb3f..48d1690665419 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/SpatialUtils.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/SpatialUtils.java @@ -7,10 +7,10 @@ package org.elasticsearch.xpack.spatial; import org.apache.lucene.util.SloppyMath; +import org.elasticsearch.index.mapper.GeoShapeIndexer; import org.elasticsearch.geometry.Circle; import org.elasticsearch.geometry.LinearRing; import org.elasticsearch.geometry.Polygon; -import org.elasticsearch.index.mapper.GeoShapeIndexer; /** * Utility class for storing different helpful re-usable spatial functions diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/fielddata/GeoShapeValues.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/fielddata/GeoShapeValues.java index f709dc7349bc3..bdec83de9bd5c 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/fielddata/GeoShapeValues.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/fielddata/GeoShapeValues.java @@ -7,11 +7,11 @@ package org.elasticsearch.xpack.spatial.index.fielddata; +import org.elasticsearch.index.mapper.GeoShapeIndexer; import org.elasticsearch.geometry.Geometry; import org.elasticsearch.geometry.Rectangle; import org.elasticsearch.geometry.utils.GeographyValidator; import org.elasticsearch.geometry.utils.WellKnownText; -import org.elasticsearch.index.mapper.GeoShapeIndexer; import org.elasticsearch.search.aggregations.support.ValuesSourceType; import org.elasticsearch.xpack.spatial.index.mapper.BinaryGeoShapeDocValuesField; import org.elasticsearch.xpack.spatial.search.aggregations.support.GeoShapeValuesSourceType; @@ -135,7 +135,7 @@ public static GeoShapeValue missing(String missing) { final GeoShapeIndexer indexer = new GeoShapeIndexer(true, "missing"); final Geometry geometry = indexer.prepareForIndexing(MISSING_GEOMETRY_PARSER.fromWKT(missing)); final BinaryGeoShapeDocValuesField field = new BinaryGeoShapeDocValuesField("missing"); - field.add(indexer.indexShape(null, geometry), geometry); + field.add(indexer.indexShape(geometry), geometry); final GeometryDocValueReader reader = new GeometryDocValueReader(); reader.reset(field.binaryValue()); return new GeoShapeValue(reader); diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapper.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapper.java index 462cd6eafe37b..c9d3b6ee3c633 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapper.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/GeoShapeWithDocValuesFieldMapper.java @@ -37,6 +37,7 @@ import org.elasticsearch.xpack.spatial.index.fielddata.plain.AbstractLatLonShapeIndexFieldData; import org.elasticsearch.xpack.spatial.search.aggregations.support.GeoShapeValuesSourceType; +import java.io.IOException; import java.util.Arrays; import java.util.List; import java.util.Map; @@ -64,7 +65,7 @@ *

* "field" : "POLYGON ((100.0 0.0, 101.0 0.0, 101.0 1.0, 100.0 1.0, 100.0 0.0)) */ -public class GeoShapeWithDocValuesFieldMapper extends AbstractShapeGeometryFieldMapper { +public class GeoShapeWithDocValuesFieldMapper extends AbstractShapeGeometryFieldMapper { public static final String CONTENT_TYPE = "geo_shape"; private static Builder builder(FieldMapper in) { @@ -120,17 +121,6 @@ public GeoShapeWithDocValuesFieldMapper build(ContentPath contentPath) { } - @Override - @SuppressWarnings({"rawtypes", "unchecked"}) - protected void addDocValuesFields(String name, Geometry shape, List fields, ParseContext context) { - BinaryGeoShapeDocValuesField docValuesField = (BinaryGeoShapeDocValuesField) context.doc().getByKey(name); - if (docValuesField == null) { - docValuesField = new BinaryGeoShapeDocValuesField(name); - context.doc().addWithKey(name, docValuesField); - } - docValuesField.add(fields, shape); - } - public static final class GeoShapeWithDocValuesFieldType extends AbstractShapeGeometryFieldType implements GeoShapeQueryable { public GeoShapeWithDocValuesFieldType(String name, boolean indexed, boolean hasDocValues, @@ -191,14 +181,35 @@ public Query geoShapeQuery(Geometry shape, String fieldName, ShapeRelation relat }; private final Builder builder; + private final GeoShapeIndexer indexer; public GeoShapeWithDocValuesFieldMapper(String simpleName, MappedFieldType mappedFieldType, MultiFields multiFields, CopyTo copyTo, GeoShapeIndexer indexer, GeoShapeParser parser, Builder builder) { super(simpleName, mappedFieldType, builder.ignoreMalformed.get(), builder.coerce.get(), builder.ignoreZValue.get(), builder.orientation.get(), - multiFields, copyTo, indexer, parser); + multiFields, copyTo, parser); this.builder = builder; + this.indexer = indexer; + } + + @Override + protected void index(ParseContext context, Geometry geometry) throws IOException { + List fields = indexer.indexShape(geometry); + if (fieldType().isSearchable()) { + context.doc().addAll(fields); + } + if (fieldType().hasDocValues()) { + String name = fieldType().name(); + BinaryGeoShapeDocValuesField docValuesField = (BinaryGeoShapeDocValuesField) context.doc().getByKey(name); + if (docValuesField == null) { + docValuesField = new BinaryGeoShapeDocValuesField(name); + context.doc().addWithKey(name, docValuesField); + } + docValuesField.add(fields, geometry); + } else if (fieldType().isSearchable()) { + createFieldNamesField(context); + } } @Override @@ -221,13 +232,4 @@ public GeoShapeWithDocValuesFieldType fieldType() { return (GeoShapeWithDocValuesFieldType) super.fieldType(); } - @Override - protected void addStoredFields(ParseContext context, Geometry geometry) { - // noop (stored fields not available for geo_shape fields) - } - - @Override - protected void addMultiFields(ParseContext context, Geometry geometry) { - // noop (completion suggester currently not compatible with geo_shape) - } } diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldMapper.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldMapper.java index 65449e03438c0..d83a5afef0687 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldMapper.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldMapper.java @@ -9,7 +9,6 @@ import org.apache.lucene.document.StoredField; import org.apache.lucene.document.XYDocValuesField; import org.apache.lucene.document.XYPointField; -import org.apache.lucene.index.IndexableField; import org.apache.lucene.search.Query; import org.elasticsearch.common.Explicit; import org.elasticsearch.common.geo.ShapeRelation; @@ -25,9 +24,8 @@ import org.elasticsearch.xpack.spatial.index.mapper.PointFieldMapper.ParsedCartesianPoint; import org.elasticsearch.xpack.spatial.index.query.ShapeQueryPointProcessor; -import java.util.ArrayList; +import java.io.IOException; import java.util.Arrays; -import java.util.Collections; import java.util.List; import java.util.Map; @@ -36,7 +34,7 @@ * * Uses lucene 8 XYPoint encoding */ -public class PointFieldMapper extends AbstractPointGeometryFieldMapper, List> { +public class PointFieldMapper extends AbstractPointGeometryFieldMapper { public static final String CONTENT_TYPE = "point"; public static class CartesianPointParser extends PointParser { @@ -99,7 +97,7 @@ public FieldMapper build(ContentPath contentPath) { PointFieldType ft = new PointFieldType(buildFullName(contentPath), indexed.get(), stored.get(), hasDocValues.get(), parser, meta.get()); return new PointFieldMapper(name, ft, multiFieldsBuilder.build(this, contentPath), - copyTo.build(), new PointIndexer(ft), parser, this); + copyTo.build(), parser, this); } } @@ -110,9 +108,9 @@ public FieldMapper build(ContentPath contentPath) { public PointFieldMapper(String simpleName, MappedFieldType mappedFieldType, MultiFields multiFields, CopyTo copyTo, - PointIndexer pointIndexer, CartesianPointParser parser, Builder builder) { + CartesianPointParser parser, Builder builder) { super(simpleName, mappedFieldType, multiFields, - builder.ignoreMalformed.get(), builder.ignoreZValue.get(), builder.nullValue.get(), copyTo, pointIndexer, parser); + builder.ignoreMalformed.get(), builder.ignoreZValue.get(), builder.nullValue.get(), copyTo, parser); this.builder = builder; } @@ -121,23 +119,18 @@ ParsedPoint nullValue() { } @Override - protected void addStoredFields(ParseContext context, List points) { - for (CartesianPoint point : points) { - context.doc().add(new StoredField(fieldType().name(), point.toString())); + protected void index(ParseContext context, ParsedCartesianPoint point) throws IOException { + if (fieldType().isSearchable()) { + context.doc().add(new XYPointField(fieldType().name(), (float) point.getX(), (float) point.getY())); } - } - - @Override - protected void addDocValuesFields(String name, List points, List fields, - ParseContext context) { - for (CartesianPoint point : points) { + if (fieldType().hasDocValues()) { context.doc().add(new XYDocValuesField(fieldType().name(), (float) point.getX(), (float) point.getY())); + } else if (fieldType().isStored() || fieldType().isSearchable()) { + createFieldNamesField(context); + } + if (fieldType().isStored()) { + context.doc().add(new StoredField(fieldType().name(), point.toString())); } - } - - @Override - protected void addMultiFields(ParseContext context, List points) { - // noop } @Override @@ -232,34 +225,4 @@ public int hashCode() { } } - protected static class PointIndexer implements Indexer, List> { - protected final PointFieldType fieldType; - - PointIndexer(PointFieldType fieldType) { - this.fieldType = fieldType; - } - - @Override - public List prepareForIndexing(List points) { - if (points == null || points.isEmpty()) { - return Collections.emptyList(); - } - return points; - } - - @Override - @SuppressWarnings("unchecked") - public Class> processedClass() { - return (Class>)(Object)List.class; - } - - @Override - public List indexShape(ParseContext context, List points) { - ArrayList fields = new ArrayList<>(1); - for (CartesianPoint point : points) { - fields.add(new XYPointField(fieldType.name(), (float) point.getX(), (float) point.getY())); - } - return fields; - } - } } diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeFieldMapper.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeFieldMapper.java index 04710afd3d213..9ae24708e685e 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeFieldMapper.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeFieldMapper.java @@ -7,7 +7,6 @@ package org.elasticsearch.xpack.spatial.index.mapper; import org.apache.lucene.document.XYShape; -import org.apache.lucene.index.IndexableField; import org.apache.lucene.search.Query; import org.elasticsearch.common.Explicit; import org.elasticsearch.common.geo.GeometryParser; @@ -23,6 +22,7 @@ import org.elasticsearch.index.query.SearchExecutionContext; import org.elasticsearch.xpack.spatial.index.query.ShapeQueryProcessor; +import java.io.IOException; import java.util.Arrays; import java.util.List; import java.util.Map; @@ -43,7 +43,7 @@ *

* "field" : "POLYGON ((1050.0 -1000.0, 1051.0 -1000.0, 1051.0 -1001.0, 1050.0 -1001.0, 1050.0 -1000.0)) */ -public class ShapeFieldMapper extends AbstractShapeGeometryFieldMapper { +public class ShapeFieldMapper extends AbstractShapeGeometryFieldMapper { public static final String CONTENT_TYPE = "shape"; private static Builder builder(FieldMapper in) { @@ -80,8 +80,7 @@ public ShapeFieldMapper build(ContentPath contentPath) { ShapeFieldType ft = new ShapeFieldType(buildFullName(contentPath), indexed.get(), orientation.get().value(), parser, meta.get()); return new ShapeFieldMapper(name, ft, - multiFieldsBuilder.build(this, contentPath), copyTo.build(), - new ShapeIndexer(ft.name()), parser, this); + multiFieldsBuilder.build(this, contentPath), copyTo.build(), parser, this); } } @@ -112,30 +111,22 @@ public String typeName() { } private final Builder builder; + private final ShapeIndexer indexer; public ShapeFieldMapper(String simpleName, MappedFieldType mappedFieldType, MultiFields multiFields, CopyTo copyTo, - Indexer indexer, Parser parser, Builder builder) { + Parser parser, Builder builder) { super(simpleName, mappedFieldType, builder.ignoreMalformed.get(), builder.coerce.get(), builder.ignoreZValue.get(), builder.orientation.get(), - multiFields, copyTo, indexer, parser); + multiFields, copyTo, parser); this.builder = builder; + this.indexer = new ShapeIndexer(mappedFieldType.name()); } @Override - protected void addStoredFields(ParseContext context, Geometry geometry) { - // noop: we currently do not store geo_shapes - // @todo store as geojson string? - } - - @Override - protected void addDocValuesFields(String name, Geometry geometry, List fields, ParseContext context) { - // we should throw a mapping exception before we get here - } - - @Override - protected void addMultiFields(ParseContext context, Geometry geometry) { - // noop (completion suggester currently not compatible with geo_shape) + protected void index(ParseContext context, Geometry geometry) throws IOException { + context.doc().addAll(indexer.indexShape(geometry)); + createFieldNamesField(context); } @Override diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeIndexer.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeIndexer.java index 437e46432e789..95998cc5001e4 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeIndexer.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/index/mapper/ShapeIndexer.java @@ -20,33 +20,24 @@ import org.elasticsearch.geometry.Point; import org.elasticsearch.geometry.Polygon; import org.elasticsearch.geometry.Rectangle; -import org.elasticsearch.index.mapper.AbstractShapeGeometryFieldMapper; -import org.elasticsearch.index.mapper.ParseContext; import org.elasticsearch.xpack.spatial.common.ShapeUtils; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; -public class ShapeIndexer implements AbstractShapeGeometryFieldMapper.Indexer { +public class ShapeIndexer { private final String name; public ShapeIndexer(String name) { this.name = name; } - @Override - public Geometry prepareForIndexing(Geometry geometry) { - return geometry; - } - - @Override - public Class processedClass() { - return Geometry.class; - } - - @Override - public List indexShape(ParseContext context, Geometry shape) { + public List indexShape(Geometry shape) { + if (shape == null) { + return Collections.emptyList(); + } LuceneGeometryVisitor visitor = new LuceneGeometryVisitor(name); shape.visit(visitor); return visitor.fields; diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/fielddata/TriangleTreeTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/fielddata/TriangleTreeTests.java index 3afa439e81a55..6bc138e95aa6b 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/fielddata/TriangleTreeTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/fielddata/TriangleTreeTests.java @@ -26,7 +26,7 @@ public void testVisitAllTriangles() throws IOException { Geometry geometry = GeometryTestUtils.randomGeometryWithoutCircle(randomIntBetween(1, 10), false); // write tree GeoShapeIndexer indexer = new GeoShapeIndexer(true, "test"); - List fieldList = indexer.indexShape(null, indexer.prepareForIndexing(geometry)); + List fieldList = indexer.indexShape(geometry); ByteBuffersDataOutput output = new ByteBuffersDataOutput(); TriangleTreeWriter.writeTo(output, fieldList); // read tree diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/CartesianFieldMapperTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/CartesianFieldMapperTests.java index 4ee7988e564f4..f986125ba954e 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/CartesianFieldMapperTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/CartesianFieldMapperTests.java @@ -145,7 +145,7 @@ public void testZValue() throws IOException { MapperParsingException e = expectThrows(MapperParsingException.class, () -> mapper2.parse(source(b -> b.field(FIELD_NAME, "POINT (2000.1 305.6 34567.33)"))) ); - assertThat(e.getMessage(), containsString("failed to parse field [" + FIELD_NAME + "] of type")); + assertThat(e.getMessage(), containsString("Failed to parse field [" + FIELD_NAME + "] of type")); assertThat(e.getRootCause().getMessage(), containsString("found Z value [34567.33] but [ignore_z_value] parameter is [false]")); } diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/LatLonShapeDocValuesQueryTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/LatLonShapeDocValuesQueryTests.java index d4908cddae277..5ad7e9832924a 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/LatLonShapeDocValuesQueryTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/LatLonShapeDocValuesQueryTests.java @@ -73,8 +73,7 @@ public void testIndexSimpleShapes() throws Exception { GeometryTestUtils::randomPolygon ); Geometry geometry = geometryFunc.apply(false); - geometry = indexer.prepareForIndexing(geometry); - List fields = indexer.indexShape(null, geometry); + List fields = indexer.indexShape(geometry); for (IndexableField field : fields) { doc.add(field); } @@ -116,8 +115,7 @@ public void testIndexMultiShapes() throws Exception { for (int id = 0; id < numDocs; id++) { Document doc = new Document(); Geometry geometry = GeometryTestUtils.randomGeometryWithoutCircle(randomIntBetween(1, 5), false); - geometry = indexer.prepareForIndexing(geometry); - List fields = indexer.indexShape(null, geometry); + List fields = indexer.indexShape(geometry); for (IndexableField field : fields) { doc.add(field); } diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldTypeTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldTypeTests.java index 87f9373f780d2..5407e3f6d323a 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldTypeTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/index/mapper/PointFieldTypeTests.java @@ -56,10 +56,9 @@ public void testFetchSourceValue() throws IOException { assertEquals(Collections.singletonList(wktPoint), fetchSourceValue(mapper, sourceValue, "wkt")); // Test a list of points in [x, y] array format with a malformed entry - // TODO Point Field parsers have a weird `ignore_malformed` impl that still throws errors - // on many types of malformed input - // sourceValue = List.of(List.of(42.0, 27.1), List.of("a", "b"), List.of(30.0, 50.0)); - // assertEquals(List.of(jsonPoint, otherJsonPoint), fetchSourceValue(mapper, sourceValue, null)); - // assertEquals(List.of(wktPoint, otherWktPoint), fetchSourceValue(mapper, sourceValue, "wkt")); + sourceValue = Arrays.asList(Arrays.asList(42.0, 27.1), Arrays.asList("a", "b"), Arrays.asList(30.0, 50.0)); + assertEquals(Arrays.asList(jsonPoint, otherJsonPoint), fetchSourceValue(mapper, sourceValue, null)); + assertEquals(Arrays.asList(wktPoint, otherWktPoint), fetchSourceValue(mapper, sourceValue, "wkt")); + } } diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/ingest/CircleProcessorTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/ingest/CircleProcessorTests.java index 0fa444fdd98ff..8d1b44487cea1 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/ingest/CircleProcessorTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/ingest/CircleProcessorTests.java @@ -226,8 +226,7 @@ public void testGeoShapeQueryAcrossDateline() throws IOException { try (Directory dir = newDirectory(); RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { Document doc = new Document(); GeoShapeIndexer indexer = new GeoShapeIndexer(true, fieldName); - Geometry normalized = indexer.prepareForIndexing(geometry); - for (IndexableField field : indexer.indexShape(null, normalized)) { + for (IndexableField field : indexer.indexShape(geometry)) { doc.add(field); } w.addDocument(doc); @@ -258,8 +257,7 @@ public void testShapeQuery() throws IOException { try (Directory dir = newDirectory(); RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { Document doc = new Document(); ShapeIndexer indexer = new ShapeIndexer(fieldName); - Geometry normalized = indexer.prepareForIndexing(geometry); - for (IndexableField field : indexer.indexShape(null, normalized)) { + for (IndexableField field : indexer.indexShape(geometry)) { doc.add(field); } w.addDocument(doc); diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/util/GeoTestUtils.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/util/GeoTestUtils.java index f6e0132647ee7..3c775b7c66652 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/util/GeoTestUtils.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/util/GeoTestUtils.java @@ -43,7 +43,7 @@ public static GeometryDocValueReader geometryDocValueReader(Geometry geometry, C CentroidCalculator centroidCalculator = new CentroidCalculator(); centroidCalculator.add(geometry); GeometryDocValueReader reader = new GeometryDocValueReader(); - reader.reset(GeometryDocValueWriter.write(indexer.indexShape(null, geometry), encoder, centroidCalculator)); + reader.reset(GeometryDocValueWriter.write(indexer.indexShape(geometry), encoder, centroidCalculator)); return reader; } @@ -51,7 +51,7 @@ public static BinaryGeoShapeDocValuesField binaryGeoShapeDocValuesField(String n GeoShapeIndexer indexer = new GeoShapeIndexer(true, name); geometry = indexer.prepareForIndexing(geometry); BinaryGeoShapeDocValuesField field = new BinaryGeoShapeDocValuesField(name); - field.add(indexer.indexShape(null, geometry) , geometry); + field.add(indexer.indexShape(geometry) , geometry); return field; }