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

Remove types from GeoShapeQueryBuilder #47792

Merged
merged 6 commits into from
Oct 23, 2019
Merged

Remove types from GeoShapeQueryBuilder #47792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
23f04b3
Remove types from GeoShapeQueryBuilder
romseygeek Oct 9, 2019
2594158
tests
romseygeek Oct 9, 2019
5f7be24
Merge branch 'master' into types-removal/abstract-geo-querybuilder
elasticmachine Oct 21, 2019
20c41c4
Merge remote-tracking branch 'origin/master' into types-removal/abstr…
romseygeek Oct 23, 2019
290d6ce
Re-add javadocs
romseygeek Oct 23, 2019
886457b
Merge remote-tracking branch 'romseygeek/types-removal/abstract-geo-q…
romseygeek Oct 23, 2019
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
90 changes: 26 additions & 64 deletions server/src/main/java/org/elasticsearch/index/query/AbstractGeometryQueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@
import org.apache.lucene.search.MatchNoDocsQuery;
import org.apache.lucene.search.Query;
import org.apache.lucene.util.SetOnce;
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.get.GetRequest;
import org.elasticsearch.action.get.GetResponse;
import org.elasticsearch.client.Client;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.geo.GeoJson;
Expand All @@ -43,6 +43,7 @@
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.geometry.Geometry;
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.mapper.MapperService;

import java.io.IOException;
import java.util.ArrayList;
Expand All @@ -55,9 +56,6 @@
*/
public abstract class AbstractGeometryQueryBuilder<QB extends AbstractGeometryQueryBuilder<QB>> extends AbstractQueryBuilder<QB> {

static final String TYPES_DEPRECATION_MESSAGE = "[types removal] Types are deprecated in [geo_shape] queries. " +
"The type should no longer be specified in the [indexed_shape] section.";

public static final String DEFAULT_SHAPE_INDEX_NAME = "shapes";
public static final String DEFAULT_SHAPE_FIELD_NAME = "shape";
public static final ShapeRelation DEFAULT_SHAPE_RELATION = ShapeRelation.INTERSECTS;
Expand All @@ -72,7 +70,6 @@ public abstract class AbstractGeometryQueryBuilder<QB extends AbstractGeometryQu
protected static final ParseField RELATION_FIELD = new ParseField("relation");
protected static final ParseField INDEXED_SHAPE_FIELD = new ParseField("indexed_shape");
protected static final ParseField SHAPE_ID_FIELD = new ParseField("id");
protected static final ParseField SHAPE_TYPE_FIELD = new ParseField("type");
protected static final ParseField SHAPE_INDEX_FIELD = new ParseField("index");
protected static final ParseField SHAPE_PATH_FIELD = new ParseField("path");
protected static final ParseField SHAPE_ROUTING_FIELD = new ParseField("routing");
Expand All @@ -82,7 +79,6 @@ public abstract class AbstractGeometryQueryBuilder<QB extends AbstractGeometryQu
protected final Supplier<Geometry> supplier;

protected final String indexedShapeId;
protected final String indexedShapeType;

protected Geometry shape;
protected String indexedShapeIndex = DEFAULT_SHAPE_INDEX_NAME;
Expand All @@ -105,7 +101,7 @@ public abstract class AbstractGeometryQueryBuilder<QB extends AbstractGeometryQu
*/
@Deprecated
protected AbstractGeometryQueryBuilder(String fieldName, ShapeBuilder shape) {
this(fieldName, shape == null ? null : shape.buildGeometry(), null, null);
this(fieldName, shape == null ? null : shape.buildGeometry(), null);
}

/**
Expand All @@ -118,7 +114,7 @@ protected AbstractGeometryQueryBuilder(String fieldName, ShapeBuilder shape) {
* Shape used in the Query
*/
public AbstractGeometryQueryBuilder(String fieldName, Geometry shape) {
this(fieldName, shape, null, null);
this(fieldName, shape, null);
}

/**
Expand All @@ -131,28 +127,10 @@ public AbstractGeometryQueryBuilder(String fieldName, Geometry shape) {
* ID of the indexed Shape that will be used in the Query
*/
protected AbstractGeometryQueryBuilder(String fieldName, String indexedShapeId) {
this(fieldName, (Geometry) null, indexedShapeId, null);
this(fieldName, (Geometry) null, indexedShapeId);
}

/**
* Creates a new AbstractGeometryQueryBuilder whose Query will be against the given
* field name and will use the Shape found with the given ID in the given
* type
*
* @param fieldName
* Name of the field that will be filtered
* @param indexedShapeId
* ID of the indexed Shape that will be used in the Query
* @param indexedShapeType
* Index type of the indexed Shapes
* @deprecated use {@link #AbstractGeometryQueryBuilder(String, String)} instead
*/
@Deprecated
protected AbstractGeometryQueryBuilder(String fieldName, String indexedShapeId, String indexedShapeType) {
this(fieldName, (Geometry) null, indexedShapeId, indexedShapeType);
}

protected AbstractGeometryQueryBuilder(String fieldName, Geometry shape, String indexedShapeId, @Nullable String indexedShapeType) {
protected AbstractGeometryQueryBuilder(String fieldName, Geometry shape, String indexedShapeId) {
if (fieldName == null) {
throw new IllegalArgumentException("fieldName is required");
}
Expand All @@ -162,17 +140,20 @@ protected AbstractGeometryQueryBuilder(String fieldName, Geometry shape, String
this.fieldName = fieldName;
this.shape = shape;
this.indexedShapeId = indexedShapeId;
this.indexedShapeType = indexedShapeType;
this.supplier = null;
}

protected AbstractGeometryQueryBuilder(String fieldName, Supplier<Geometry> supplier, String indexedShapeId,
@Nullable String indexedShapeType) {
protected AbstractGeometryQueryBuilder(String fieldName, Supplier<Geometry> supplier, String indexedShapeId) {
if (fieldName == null) {
throw new IllegalArgumentException("fieldName is required");
}
if (supplier == null && indexedShapeId == null) {
throw new IllegalArgumentException("either shape or indexedShapeId is required");
}
this.fieldName = fieldName;
this.shape = null;
this.supplier = supplier;
this.indexedShapeId = indexedShapeId;
this.indexedShapeType = indexedShapeType;
this.indexedShapeId = indexedShapeId;;
}

/**
Expand All @@ -184,11 +165,13 @@ protected AbstractGeometryQueryBuilder(StreamInput in) throws IOException {
if (in.readBoolean()) {
shape = GeometryIO.readGeometry(in);
indexedShapeId = null;
indexedShapeType = null;
} else {
shape = null;
indexedShapeId = in.readOptionalString();
indexedShapeType = in.readOptionalString();
if (in.getVersion().before(Version.V_8_0_0)) {
String type = in.readOptionalString();
assert MapperService.SINGLE_MAPPING_NAME.equals(type) : "Expected type [_doc], got [" + type + "]";
}
indexedShapeIndex = in.readOptionalString();
indexedShapePath = in.readOptionalString();
indexedShapeRouting = in.readOptionalString();
Expand All @@ -210,7 +193,9 @@ protected void doWriteTo(StreamOutput out) throws IOException {
GeometryIO.writeGeometry(out, shape);
} else {
out.writeOptionalString(indexedShapeId);
out.writeOptionalString(indexedShapeType);
if (out.getVersion().before(Version.V_8_0_0)) {
out.writeOptionalString(MapperService.SINGLE_MAPPING_NAME);
}
out.writeOptionalString(indexedShapeIndex);
out.writeOptionalString(indexedShapePath);
out.writeOptionalString(indexedShapeRouting);
Expand Down Expand Up @@ -254,17 +239,6 @@ public String indexedShapeId() {
return indexedShapeId;
}

/**
* @return the document type of the indexed Shape that will be used in the
* Query
*
* @deprecated Types are in the process of being removed.
*/
@Deprecated
public String indexedShapeType() {
return indexedShapeType;
}

/**
* Sets the name of the index where the indexed Shape can be found
*
Expand Down Expand Up @@ -372,9 +346,9 @@ public boolean ignoreUnmapped() {
protected abstract void doShapeQueryXContent(XContentBuilder builder, Params params) throws IOException;
/** creates a new ShapeQueryBuilder from the provided field name and shape builder */
protected abstract AbstractGeometryQueryBuilder<QB> newShapeQueryBuilder(String fieldName, Geometry shape);
/** creates a new ShapeQueryBuilder from the provided field name, supplier, indexed shape id, and indexed shape type */
/** creates a new ShapeQueryBuilder from the provided field name, supplier and indexed shape id*/
protected abstract AbstractGeometryQueryBuilder<QB> newShapeQueryBuilder(String fieldName, Supplier<Geometry> shapeSupplier,
String indexedShapeId, String indexedShapeType);
String indexedShapeId);

/** returns true if the provided field type is valid for this query */
protected boolean isValidContentType(String typeName) {
Expand Down Expand Up @@ -469,9 +443,6 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep
} else {
builder.startObject(INDEXED_SHAPE_FIELD.getPreferredName())
.field(SHAPE_ID_FIELD.getPreferredName(), indexedShapeId);
if (indexedShapeType != null) {
builder.field(SHAPE_TYPE_FIELD.getPreferredName(), indexedShapeType);
}
if (indexedShapeIndex != null) {
builder.field(SHAPE_INDEX_FIELD.getPreferredName(), indexedShapeIndex);
}
Expand Down Expand Up @@ -503,7 +474,6 @@ protected boolean doEquals(AbstractGeometryQueryBuilder other) {
&& Objects.equals(indexedShapeId, other.indexedShapeId)
&& Objects.equals(indexedShapeIndex, other.indexedShapeIndex)
&& Objects.equals(indexedShapePath, other.indexedShapePath)
&& Objects.equals(indexedShapeType, other.indexedShapeType)
&& Objects.equals(indexedShapeRouting, other.indexedShapeRouting)
&& Objects.equals(relation, other.relation)
&& Objects.equals(shape, other.shape)
Expand All @@ -514,7 +484,7 @@ protected boolean doEquals(AbstractGeometryQueryBuilder other) {
@Override
protected int doHashCode() {
return Objects.hash(fieldName, indexedShapeId, indexedShapeIndex,
indexedShapePath, indexedShapeType, indexedShapeRouting, relation, shape, ignoreUnmapped, supplier);
indexedShapePath, indexedShapeRouting, relation, shape, ignoreUnmapped, supplier);
}

@Override
Expand All @@ -524,19 +494,14 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws
} else if (this.shape == null) {
SetOnce<Geometry> supplier = new SetOnce<>();
queryRewriteContext.registerAsyncAction((client, listener) -> {
GetRequest getRequest;
if (indexedShapeType == null) {
getRequest = new GetRequest(indexedShapeIndex, indexedShapeId);
} else {
getRequest = new GetRequest(indexedShapeIndex, indexedShapeId);
}
GetRequest getRequest = new GetRequest(indexedShapeIndex, indexedShapeId);
getRequest.routing(indexedShapeRouting);
fetch(client, getRequest, indexedShapePath, ActionListener.wrap(builder-> {
supplier.set(builder);
listener.onResponse(null);
}, listener::onFailure));
});
return newShapeQueryBuilder(this.fieldName, supplier::get, this.indexedShapeId, this.indexedShapeType).relation(relation);
return newShapeQueryBuilder(this.fieldName, supplier::get, this.indexedShapeId).relation(relation);
}
return this;
}
Expand All @@ -548,7 +513,6 @@ protected abstract static class ParsedGeometryQueryParams {
public ShapeBuilder shape;

public String id = null;
public String type = null;
public String index = null;
public String shapePath = null;
public String shapeRouting = null;
Expand Down Expand Up @@ -592,8 +556,6 @@ public static ParsedGeometryQueryParams parsedParamsFromXContent(XContentParser
} else if (token.isValue()) {
if (SHAPE_ID_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
params.id = parser.text();
} else if (SHAPE_TYPE_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
params.type = parser.text();
} else if (SHAPE_INDEX_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
params.index = parser.text();
} else if (SHAPE_PATH_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
Expand Down
34 changes: 9 additions & 25 deletions server/src/main/java/org/elasticsearch/index/query/GeoShapeQueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import org.apache.logging.log4j.LogManager;
import org.apache.lucene.search.ConstantScoreQuery;
import org.apache.lucene.search.Query;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.geo.ShapeRelation;
Expand Down Expand Up @@ -86,27 +85,15 @@ public GeoShapeQueryBuilder(String fieldName, ShapeBuilder shape) {
super(fieldName, shape);
}

public GeoShapeQueryBuilder(String fieldName, Supplier<Geometry> shapeSupplier, String indexedShapeId,
@Nullable String indexedShapeType) {
super(fieldName, shapeSupplier, indexedShapeId, indexedShapeType);
}

/**
* Creates a new GeoShapeQueryBuilder whose Query will be against the given
* field name and will use the Shape found with the given ID in the given
* type
*
* @param fieldName
* Name of the field that will be filtered
* @param indexedShapeId
* ID of the indexed Shape that will be used in the Query
* @param indexedShapeType
* Index type of the indexed Shapes
* @deprecated use {@link #GeoShapeQueryBuilder(String, String)} instead
* field name and will use the Shape found with the given shape id and supplier
* @param fieldName Name of the field that will be queried
* @param shapeSupplier A shape supplier
* @param indexedShapeId The indexed id of a shape
*/
Copy link
Member

Choose a reason for hiding this comment

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

Maybe keep and adapt the docs? Or do you think its not useful or self-explanatory?

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 added this back in, had removed it by mistake. Thanks!

@Deprecated
public GeoShapeQueryBuilder(String fieldName, String indexedShapeId, String indexedShapeType) {
super(fieldName, indexedShapeId, indexedShapeType);
public GeoShapeQueryBuilder(String fieldName, Supplier<Geometry> shapeSupplier, String indexedShapeId) {
super(fieldName, shapeSupplier, indexedShapeId);
}

/**
Expand Down Expand Up @@ -204,8 +191,8 @@ protected GeoShapeQueryBuilder newShapeQueryBuilder(String fieldName, Geometry s

@Override
protected GeoShapeQueryBuilder newShapeQueryBuilder(String fieldName, Supplier<Geometry> shapeSupplier,
String indexedShapeId, String indexedShapeType) {
return new GeoShapeQueryBuilder(fieldName, shapeSupplier, indexedShapeId, indexedShapeType);
String indexedShapeId) {
return new GeoShapeQueryBuilder(fieldName, shapeSupplier, indexedShapeId);
}

@Override
Expand Down Expand Up @@ -265,14 +252,11 @@ public static GeoShapeQueryBuilder fromXContent(XContentParser parser) throws IO
(ParsedGeoShapeQueryParams) AbstractGeometryQueryBuilder.parsedParamsFromXContent(parser, new ParsedGeoShapeQueryParams());

GeoShapeQueryBuilder builder;
if (pgsqp.type != null) {
deprecationLogger.deprecatedAndMaybeLog("geo_share_query_with_types", TYPES_DEPRECATION_MESSAGE);
}

if (pgsqp.shape != null) {
builder = new GeoShapeQueryBuilder(pgsqp.fieldName, pgsqp.shape);
} else {
builder = new GeoShapeQueryBuilder(pgsqp.fieldName, pgsqp.id, pgsqp.type);
builder = new GeoShapeQueryBuilder(pgsqp.fieldName, pgsqp.id);
}

if (pgsqp.index != null) {
Expand Down
12 changes: 2 additions & 10 deletions server/src/test/java/org/elasticsearch/index/query/GeoShapeQueryBuilderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
public class GeoShapeQueryBuilderTests extends AbstractQueryTestCase<GeoShapeQueryBuilder> {

protected static String indexedShapeId;
protected static String indexedShapeType;
protected static String indexedShapePath;
protected static String indexedShapeIndex;
protected static String indexedShapeRouting;
Expand Down Expand Up @@ -94,8 +93,7 @@ protected GeoShapeQueryBuilder doCreateTestQueryBuilder(boolean indexedShape) {
} else {
indexedShapeToReturn = shape;
indexedShapeId = randomAlphaOfLengthBetween(3, 20);
indexedShapeType = randomBoolean() ? randomAlphaOfLengthBetween(3, 20) : null;
builder = new GeoShapeQueryBuilder(fieldName(), indexedShapeId, indexedShapeType);
builder = new GeoShapeQueryBuilder(fieldName(), indexedShapeId);
if (randomBoolean()) {
indexedShapeIndex = randomAlphaOfLengthBetween(3, 20);
builder.indexedShapeIndex(indexedShapeIndex);
Expand Down Expand Up @@ -154,7 +152,6 @@ protected GetResponse executeGet(GetRequest getRequest) {
public void clearShapeFields() {
indexedShapeToReturn = null;
indexedShapeId = null;
indexedShapeType = null;
indexedShapePath = null;
indexedShapeIndex = null;
indexedShapeRouting = null;
Expand All @@ -181,7 +178,7 @@ public void testNoShape() throws IOException {

public void testNoIndexedShape() throws IOException {
IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> new GeoShapeQueryBuilder(fieldName(), null, "type"));
() -> new GeoShapeQueryBuilder(fieldName(), null, null));
assertEquals("either shape or indexedShapeId is required", e.getMessage());
}

Expand Down Expand Up @@ -297,11 +294,6 @@ public void testSerializationFailsUnlessFetched() throws IOException {
protected QueryBuilder parseQuery(XContentParser parser) throws IOException {
QueryBuilder query = super.parseQuery(parser);
assertThat(query, instanceOf(GeoShapeQueryBuilder.class));

GeoShapeQueryBuilder shapeQuery = (GeoShapeQueryBuilder) query;
if (shapeQuery.indexedShapeType() != null) {
assertWarnings(GeoShapeQueryBuilder.TYPES_DEPRECATION_MESSAGE);
}
return query;
}
}
Loading