Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Fix handling of points_only with term strategy in geo_shape #31766

Merged
merged 1 commit into from
Jul 5, 2018
Merged

Fix handling of points_only with term strategy in geo_shape #31766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 20 additions & 7 deletions server/src/main/java/org/elasticsearch/index/mapper/GeoShapeFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ public static class TypeParser implements Mapper.TypeParser {
@Override
public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext parserContext) throws MapperParsingException {
Builder builder = new Builder(name);
Boolean pointsOnly = null;
for (Iterator<Map.Entry<String, Object>> iterator = node.entrySet().iterator(); iterator.hasNext();) {
Map.Entry<String, Object> entry = iterator.next();
String fieldName = entry.getKey();
Expand Down Expand Up @@ -230,13 +231,18 @@ public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext
} else if (GeoPointFieldMapper.Names.IGNORE_Z_VALUE.getPreferredName().equals(fieldName)) {
builder.ignoreZValue(XContentMapValues.nodeBooleanValue(fieldNode, name + "." + GeoPointFieldMapper.Names.IGNORE_Z_VALUE.getPreferredName()));
iterator.remove();
} else if (Names.STRATEGY_POINTS_ONLY.equals(fieldName)
&& builder.fieldType().strategyName.equals(SpatialStrategy.TERM.getStrategyName()) == false) {
boolean pointsOnly = XContentMapValues.nodeBooleanValue(fieldNode, name + "." + Names.STRATEGY_POINTS_ONLY);
builder.fieldType().setPointsOnly(pointsOnly);
} else if (Names.STRATEGY_POINTS_ONLY.equals(fieldName)) {
pointsOnly = XContentMapValues.nodeBooleanValue(fieldNode, name + "." + Names.STRATEGY_POINTS_ONLY);
iterator.remove();
}
}
if (pointsOnly != null) {
if (builder.fieldType().strategyName.equals(SpatialStrategy.TERM.getStrategyName()) && pointsOnly == false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

pointsOnly == false is using reference equality on an object, it works because Boolean.valueOf(boolean) returns singletons, but let's still try to compare the wrapped boolean instead?

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 might be wrong, but I was under impression that this changed recently (around v1.5). According to the spec "If one of the operands is of type Boolean, it is subjected to unboxing conversion." So, I don't think we have a reference equality, but instead auto-unboxing of pointsOnly and boolean comparison with false. I also looked at the bytecode for this line and if I am reading it correctly it looks like unboxing and jump if nonzero.

    LINENUMBER 240 L40
    ALOAD 4
    INVOKEVIRTUAL org/elasticsearch/index/mapper/GeoShapeFieldMapper$Builder.fieldType ()Lorg/elasticsearch/index/mapper/GeoShapeFieldMapper$GeoShapeFieldType;
    INVOKESTATIC org/elasticsearch/index/mapper/GeoShapeFieldMapper$GeoShapeFieldType.access$000 (Lorg/elasticsearch/index/mapper/GeoShapeFieldMapper$GeoShapeFieldType;)Ljava/lang/String;
    GETSTATIC org/elasticsearch/common/geo/SpatialStrategy.TERM : Lorg/elasticsearch/common/geo/SpatialStrategy;
    INVOKEVIRTUAL org/elasticsearch/common/geo/SpatialStrategy.getStrategyName ()Ljava/lang/String;
    INVOKEVIRTUAL java/lang/String.equals (Ljava/lang/Object;)Z
    IFEQ L41
    ALOAD 5
    INVOKEVIRTUAL java/lang/Boolean.booleanValue ()Z
    IFNE L41

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know that, thanks for checking!

throw new IllegalArgumentException("points_only cannot be set to false for term strategy");
} else {
builder.fieldType().setPointsOnly(pointsOnly);
}
}
return builder;
}
}
Expand Down Expand Up @@ -565,7 +571,7 @@ protected void doXContentBody(XContentBuilder builder, boolean includeDefaults,
} else if (includeDefaults && fieldType().treeLevels() == 0) { // defaults only make sense if tree levels are not specified
builder.field(Names.TREE_PRESISION, DistanceUnit.METERS.toString(50));
}
if (includeDefaults || fieldType().strategyName() != Defaults.STRATEGY) {
if (includeDefaults || fieldType().strategyName().equals(Defaults.STRATEGY) == false) {
builder.field(Names.STRATEGY, fieldType().strategyName());
}
if (includeDefaults || fieldType().distanceErrorPct() != fieldType().defaultDistanceErrorPct) {
Expand All @@ -574,8 +580,15 @@ protected void doXContentBody(XContentBuilder builder, boolean includeDefaults,
if (includeDefaults || fieldType().orientation() != Defaults.ORIENTATION) {
builder.field(Names.ORIENTATION, fieldType().orientation());
}
if (includeDefaults || fieldType().pointsOnly() != GeoShapeFieldMapper.Defaults.POINTS_ONLY) {
builder.field(Names.STRATEGY_POINTS_ONLY, fieldType().pointsOnly());
if (fieldType().strategyName().equals(SpatialStrategy.TERM.getStrategyName())) {
// For TERMs strategy the defaults for points only change to true
if (includeDefaults || fieldType().pointsOnly() != true) {
builder.field(Names.STRATEGY_POINTS_ONLY, fieldType().pointsOnly());
}
} else {
if (includeDefaults || fieldType().pointsOnly() != GeoShapeFieldMapper.Defaults.POINTS_ONLY) {
builder.field(Names.STRATEGY_POINTS_ONLY, fieldType().pointsOnly());
}
}
if (includeDefaults || coerce.explicit()) {
builder.field(Names.COERCE, coerce.value());
Expand Down
60 changes: 58 additions & 2 deletions server/src/test/java/org/elasticsearch/index/mapper/GeoShapeFieldMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.not;

public class GeoShapeFieldMapperTests extends ESSingleNodeTestCase {

Expand Down Expand Up @@ -588,10 +589,65 @@ public void testSerializeDefaults() throws Exception {
}
}

public String toXContentString(GeoShapeFieldMapper mapper) throws IOException {
public void testPointsOnlyDefaultsWithTermStrategy() throws IOException {
String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type1")
.startObject("properties").startObject("location")
.field("type", "geo_shape")
.field("tree", "quadtree")
.field("precision", "10m")
.field("strategy", "term")
.endObject().endObject()
.endObject().endObject());

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

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

assertThat(strategy.getDistErrPct(), equalTo(0.0));
assertThat(strategy.getGrid(), instanceOf(QuadPrefixTree.class));
assertThat(strategy.getGrid().getMaxLevels(), equalTo(23));
assertThat(strategy.isPointsOnly(), equalTo(true));
// term strategy changes the default for points_only, check that we handle it correctly
assertThat(toXContentString(geoShapeFieldMapper, false), not(containsString("points_only")));
}


public void testPointsOnlyFalseWithTermStrategy() throws Exception {
String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type1")
.startObject("properties").startObject("location")
.field("type", "geo_shape")
.field("tree", "quadtree")
.field("precision", "10m")
.field("strategy", "term")
.field("points_only", false)
.endObject().endObject()
.endObject().endObject());

DocumentMapperParser parser = createIndex("test").mapperService().documentMapperParser();

IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> parser.parse("type1", new CompressedXContent(mapping))
);
assertThat(e.getMessage(), containsString("points_only cannot be set to false for term strategy"));
}

public String toXContentString(GeoShapeFieldMapper mapper, boolean includeDefaults) throws IOException {
XContentBuilder builder = XContentFactory.jsonBuilder().startObject();
mapper.doXContentBody(builder, true, new ToXContent.MapParams(Collections.singletonMap("include_defaults", "true")));
ToXContent.Params params;
if (includeDefaults) {
params = new ToXContent.MapParams(Collections.singletonMap("include_defaults", "true"));
} else {
params = ToXContent.EMPTY_PARAMS;
}
mapper.doXContentBody(builder, includeDefaults, params);
return Strings.toString(builder.endObject());
}

public String toXContentString(GeoShapeFieldMapper mapper) throws IOException {
return toXContentString(mapper, true);
}

}