-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
Changes from all commits
9d48343
6ef3e68
61de96c
9fcd17d
db49a69
393631d
76feae0
9e1307a
b4d892f
ff0269d
c49b627
b53b33b
ec447e9
c62cbd8
7bddf07
d731542
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||
|
||
String malformedException = null; | ||
|
||
|
@@ -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"); | ||
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
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 ofterm
as a strategy, and therefore ending up falling back onnull
here, which was getting translated torecursive
once the mapper had been parsed. Silently falling back torecursive
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-spelledterm
strategies will have to stay usingrecursive
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.