Skip to content

Convert geo field mappers to Parametrized form #63836

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

Merged
merged 16 commits into from
Oct 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions docs/reference/migration/migrate_8_0/mappings.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,21 @@ For a detailed migration guide, see the {ref}/migrate-to-java-time.html[Java
time migration guide].
====
// end::notable-breaking-changes[]

[[geo-shape-strategy]]
.The `strategy` parameter on `geo_shape` mappings now rejects unknown strategies
[%collapsible]
====
*Details* +
The only permissible values for the `strategy` parameter on `geo_shape` mappings
are `term` and `recursive`. In 7.x if a non-permissible value was used as a
parameter here, the mapping would silently fall back to using `recursive`. The
mapping will now be rejected instead.

*Impact* +
This will have no impact on existing mappings created with non-permissible
strategy values, as they will already be serializing themselves as if they
had been configured as `recursive`. New indexes will need to use one of the
permissible strategies, or preferably not define a strategy at all and use
the far more efficient BKD index.
====
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,6 @@ public class GeoPlugin extends Plugin implements MapperPlugin {

@Override
public Map<String, Mapper.TypeParser> getMappers() {
return Collections.singletonMap(GeoShapeFieldMapper.CONTENT_TYPE, new GeoShapeFieldMapper.TypeParser());
return Collections.singletonMap(GeoShapeFieldMapper.CONTENT_TYPE, GeoShapeFieldMapper.PARSER);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ public void testMappingUpdate() throws Exception {
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> client().admin().indices()
.preparePutMapping("test")
.setSource(update, XContentType.JSON).get());
assertThat(e.getMessage(), containsString("using [BKD] strategy cannot be merged with"));
assertThat(e.getMessage(), containsString("mapper [shape] of type [geo_shape] cannot change strategy from [BKD] to [recursive]"));
}

/**
Expand Down
23 changes: 21 additions & 2 deletions server/src/main/java/org/elasticsearch/common/Strings.java
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,25 @@ public static String[] toStringArray(Collection<String> collection) {
return collection.toArray(new String[collection.size()]);
}

/**
* Concatenate two string arrays into a third
*/
public static String[] concatStringArrays(String[] first, String[] second) {
if (first == null && second == null) {
return Strings.EMPTY_ARRAY;
}
if (first == null || first.length == 0) {
return second;
}
if (second == null || second.length == 0) {
return first;
}
String[] concat = new String[first.length + second.length];
System.arraycopy(first, 0, concat, 0, first.length);
System.arraycopy(second, 0, concat, first.length, second.length);
return concat;
}

/**
* Tokenize the specified string by commas to a set, trimming whitespace and ignoring empty tokens.
*
Expand Down Expand Up @@ -879,7 +898,7 @@ public static String padStart(String s, int minimumLength, char c) {
return sb.toString();
}
}

public static String toLowercaseAscii(String in) {
StringBuilder out = new StringBuilder();
Iterator<Integer> iter = in.codePoints().iterator();
Expand All @@ -892,5 +911,5 @@ public static String toLowercaseAscii(String in) {
}
}
return out.toString();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,10 @@
import org.apache.lucene.index.IndexableField;
import org.apache.lucene.util.BitUtil;
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.geo.GeoUtils.EffectivePoint;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.geometry.Geometry;
import org.elasticsearch.geometry.Point;
import org.elasticsearch.geometry.Rectangle;
Expand All @@ -41,8 +41,6 @@
import java.util.Arrays;
import java.util.Locale;

import static org.elasticsearch.index.mapper.AbstractPointGeometryFieldMapper.Names.IGNORE_Z_VALUE;

public class GeoPoint implements ToXContentFragment {

protected double lat;
Expand Down Expand Up @@ -265,8 +263,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws

public static double assertZValue(final boolean ignoreZValue, double zValue) {
if (ignoreZValue == false) {
throw new ElasticsearchParseException("Exception parsing coordinates: found Z value [{}] but [{}] "
+ "parameter is [{}]", zValue, IGNORE_Z_VALUE, ignoreZValue);
throw new ElasticsearchParseException("Exception parsing coordinates: found Z value [{}] but [ignore_z_value] "
+ "parameter is [{}]", zValue, ignoreZValue);
}
return zValue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,6 @@ public static SpatialStrategy fromString(String strategyName) {
return strategy;
}
}
return null;
throw new IllegalArgumentException("Unknown strategy [" + strategyName + "]");
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ended up catching a pre-existing bug in GeoShapeQueryTests, which was asking for TERM instead of term as a strategy, and therefore ending up falling back on null here, which was getting translated to recursive once the mapper had been parsed. Silently falling back to recursive is a bug, I think, although on backport we will need to change this to emit a warning instead, as you can't change the strategy once it has been set and so existing mappings with badly-spelled term strategies will have to stay using recursive

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this is a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say it was a bugfix rather than a breaking change, I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #63975 to emit a deprecation warning for this case in 7.x

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing mappings sort of fix themselves, btw, so my above comment was not correct. Once a user has added a mapping with a bad strategy, it will serialize itself as recursive, so we don't need to worry about parsing mappings from previous versions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is what I checked so the only change is that creating a mapping will throw an error when before it was successful.

}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
package org.elasticsearch.common.geo.parsers;

import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.Explicit;
import org.elasticsearch.common.geo.GeoPoint;
import org.elasticsearch.common.geo.GeoShapeType;
import org.elasticsearch.common.geo.builders.CircleBuilder;
Expand Down Expand Up @@ -50,14 +49,10 @@ protected static ShapeBuilder parse(XContentParser parser, AbstractShapeGeometry
GeometryCollectionBuilder geometryCollections = null;

Orientation orientation = (shapeMapper == null)
? AbstractShapeGeometryFieldMapper.Defaults.ORIENTATION.value()
? Orientation.RIGHT
: shapeMapper.orientation();
Explicit<Boolean> coerce = (shapeMapper == null)
? AbstractShapeGeometryFieldMapper.Defaults.COERCE
: shapeMapper.coerce();
Explicit<Boolean> ignoreZValue = (shapeMapper == null)
? AbstractShapeGeometryFieldMapper.Defaults.IGNORE_Z_VALUE
: shapeMapper.ignoreZValue();
boolean coerce = shapeMapper != null && shapeMapper.coerce();
boolean ignoreZValue = shapeMapper == null || shapeMapper.ignoreZValue();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether or not a parameter has been explicitly set is of interest only to the mapper itself, and its serialization code; the PR changes these getters on the mappers to return simple booleans.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1


String malformedException = null;

Expand All @@ -78,7 +73,7 @@ protected static ShapeBuilder parse(XContentParser parser, AbstractShapeGeometry
}
} else if (ShapeParser.FIELD_COORDINATES.match(fieldName, subParser.getDeprecationHandler())) {
subParser.nextToken();
CoordinateNode tempNode = parseCoordinates(subParser, ignoreZValue.value());
CoordinateNode tempNode = parseCoordinates(subParser, ignoreZValue);
if (coordinateNode != null && tempNode.numDimensions() != coordinateNode.numDimensions()) {
throw new ElasticsearchParseException("Exception parsing coordinates: " +
"number of dimensions do not match");
Expand Down Expand Up @@ -134,7 +129,7 @@ protected static ShapeBuilder parse(XContentParser parser, AbstractShapeGeometry
return geometryCollections;
}

return shapeType.getBuilder(coordinateNode, radius, orientation, coerce.value());
return shapeType.getBuilder(coordinateNode, radius, orientation, coerce);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
package org.elasticsearch.common.geo.parsers;

import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.Explicit;
import org.elasticsearch.common.geo.GeoPoint;
import org.elasticsearch.common.geo.GeoShapeType;
import org.elasticsearch.common.geo.builders.CoordinatesBuilder;
Expand Down Expand Up @@ -78,9 +77,8 @@ public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoSha
final AbstractShapeGeometryFieldMapper shapeMapper)
throws IOException, ElasticsearchParseException {
try (StringReader reader = new StringReader(parser.text())) {
Explicit<Boolean> ignoreZValue = (shapeMapper == null) ? AbstractShapeGeometryFieldMapper.Defaults.IGNORE_Z_VALUE :
shapeMapper.ignoreZValue();
Explicit<Boolean> coerce = (shapeMapper == null) ? AbstractShapeGeometryFieldMapper.Defaults.COERCE : shapeMapper.coerce();
boolean coerce = shapeMapper != null && shapeMapper.coerce();
boolean ignoreZValue = shapeMapper == null || shapeMapper.ignoreZValue();
// setup the tokenizer; configured to read words w/o numbers
StreamTokenizer tokenizer = new StreamTokenizer(reader);
tokenizer.resetSyntax();
Expand All @@ -93,7 +91,7 @@ public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoSha
tokenizer.wordChars('.', '.');
tokenizer.whitespaceChars(0, ' ');
tokenizer.commentChar('#');
ShapeBuilder builder = parseGeometry(tokenizer, shapeType, ignoreZValue.value(), coerce.value());
ShapeBuilder builder = parseGeometry(tokenizer, shapeType, ignoreZValue, coerce);
checkEOF(tokenizer);
return builder;
}
Expand Down Expand Up @@ -258,7 +256,7 @@ private static PolygonBuilder parsePolygon(StreamTokenizer stream, final boolean
return null;
}
PolygonBuilder builder = new PolygonBuilder(parseLinearRing(stream, ignoreZValue, coerce),
AbstractShapeGeometryFieldMapper.Defaults.ORIENTATION.value());
ShapeBuilder.Orientation.RIGHT);
while (nextCloserOrComma(stream).equals(COMMA)) {
builder.hole(parseLinearRing(stream, ignoreZValue, coerce));
}
Expand Down
Loading