Skip to content

Remove types from Graph API #46935

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

Merged
merged 2 commits into from
Sep 23, 2019
Merged

Remove types from Graph API #46935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
22d0cf6
Remove types from Graph API
romseygeek Sep 20, 2019
d8775c8
feedback; also remove from RHLC
romseygeek Sep 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
2 changes: 1 addition & 1 deletion client/rest-high-level/src/main/java/org/elasticsearch/client/GraphRequestConverters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ final class GraphRequestConverters {
private GraphRequestConverters() {}

static Request explore(GraphExploreRequest exploreRequest) throws IOException {
String endpoint = RequestConverters.endpoint(exploreRequest.indices(), exploreRequest.types(), "_graph/explore");
String endpoint = RequestConverters.endpoint(exploreRequest.indices(), "_graph/explore");
Request request = new Request(HttpGet.METHOD_NAME, endpoint);
request.setEntity(RequestConverters.createEntity(exploreRequest, RequestConverters.REQUEST_BODY_CONTENT_TYPE));
return request;
Expand Down
40 changes: 7 additions & 33 deletions client/rest-high-level/src/main/java/org/elasticsearch/client/graph/GraphExploreRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ public class GraphExploreRequest implements IndicesRequest.Replaceable, ToXConte
public static final String NO_VERTICES_ERROR_MESSAGE = "Graph explore hop must have at least one VertexRequest";
private String[] indices = Strings.EMPTY_ARRAY;
private IndicesOptions indicesOptions = IndicesOptions.fromOptions(false, false, true, false);
private String[] types = Strings.EMPTY_ARRAY;
private String routing;
private TimeValue timeout;

Expand Down Expand Up @@ -106,31 +105,6 @@ public GraphExploreRequest indicesOptions(IndicesOptions indicesOptions) {
return this;
}

/**
* The document types to execute the explore against. Defaults to be executed against
* all types.
*
* @deprecated Types are in the process of being removed. Instead of using a type, prefer to
* filter on a field on the document.
*/
@Deprecated
public String[] types() {
return this.types;
}

/**
* The document types to execute the explore request against. Defaults to be executed against
* all types.
*
* @deprecated Types are in the process of being removed. Instead of using a type, prefer to
* filter on a field on the document.
*/
@Deprecated
public GraphExploreRequest types(String... types) {
this.types = types;
return this;
}

public String routing() {
return this.routing;
}
Expand All @@ -154,7 +128,7 @@ public TimeValue timeout() {
* operations involved in each hop are limited to the remaining time
* available but can still overrun due to the nature of their "best efforts"
* timeout support. When a timeout occurs partial results are returned.
*
*
* @param timeout
* a {@link TimeValue} object which determines the maximum length
* of time to spend exploring
Expand All @@ -174,7 +148,7 @@ public GraphExploreRequest timeout(String timeout) {

@Override
public String toString() {
return "graph explore [" + Arrays.toString(indices) + "][" + Arrays.toString(types) + "]";
return "graph explore [" + Arrays.toString(indices) + "]";
}

/**
Expand All @@ -190,7 +164,7 @@ public String toString() {
* better with smaller samples as there are less look-ups required for
* background frequencies of terms found in the documents
* </p>
*
*
* @param maxNumberOfDocsPerHop
* shard-level sample size in documents
*/
Expand Down Expand Up @@ -231,7 +205,7 @@ public int maxDocsPerDiversityValue() {
* default value is true which means terms are selected based on
* significance (see the {@link SignificantTerms} aggregation) rather than
* popularity (using the {@link TermsAggregator}).
*
*
* @param value
* true if the significant_terms algorithm should be used.
*/
Expand All @@ -246,7 +220,7 @@ public boolean useSignificance() {
/**
* Return detailed information about vertex frequencies as part of JSON
* results - defaults to false
*
*
* @param value
* true if detailed information is required in JSON responses
*/
Expand All @@ -262,7 +236,7 @@ public boolean returnDetailedInfo() {
* Add a stage in the graph exploration. Each hop represents a stage of
* querying elasticsearch to identify terms which can then be connnected to
* other terms in a subsequent hop.
*
*
* @param guidingQuery
* optional choice of query which influences which documents are
* considered in this stage
Expand Down Expand Up @@ -316,7 +290,7 @@ public float getBoost() {
@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();

builder.startObject("controls");
{
if (sampleSize != SamplerAggregationBuilder.DEFAULT_SHARD_SAMPLE_SIZE) {
Expand Down
5 changes: 2 additions & 3 deletions ...ch/client/GrapRequestConvertersTests.java → ...h/client/GraphRequestConvertersTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,14 @@

import static org.hamcrest.Matchers.is;

public class GrapRequestConvertersTests extends ESTestCase {
public class GraphRequestConvertersTests extends ESTestCase {

public void testGraphExplore() throws Exception {
Map<String, String> expectedParams = new HashMap<>();

GraphExploreRequest graphExploreRequest = new GraphExploreRequest();
graphExploreRequest.sampleDiversityField("diversity");
graphExploreRequest.indices("index1", "index2");
graphExploreRequest.types("type1", "type2");
int timeout = randomIntBetween(10000, 20000);
graphExploreRequest.timeout(TimeValue.timeValueMillis(timeout));
graphExploreRequest.useSignificance(randomBoolean());
Expand All @@ -58,7 +57,7 @@ public void testGraphExplore() throws Exception {
}
Request request = GraphRequestConverters.explore(graphExploreRequest);
assertEquals(HttpGet.METHOD_NAME, request.getMethod());
assertEquals("/index1,index2/type1,type2/_graph/explore", request.getEndpoint());
assertEquals("/index1,index2/_graph/explore", request.getEndpoint());
assertEquals(expectedParams, request.getParameters());
assertThat(request.getEntity().getContentType().getValue(), is(XContentType.JSON.mediaTypeWithoutParameters()));
RequestConvertersTests.assertToXContentBody(graphExploreRequest, request.getEntity());
Expand Down
54 changes: 16 additions & 38 deletions ...plugin/core/src/main/java/org/elasticsearch/protocol/xpack/graph/GraphExploreRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.protocol.xpack.graph;

import org.elasticsearch.Version;
import org.elasticsearch.action.ActionRequest;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.IndicesRequest;
Expand All @@ -24,7 +25,6 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;

/**
Expand All @@ -37,7 +37,6 @@ public class GraphExploreRequest extends ActionRequest implements IndicesRequest
public static final String NO_VERTICES_ERROR_MESSAGE = "Graph explore hop must have at least one VertexRequest";
private String[] indices = Strings.EMPTY_ARRAY;
private IndicesOptions indicesOptions = IndicesOptions.fromOptions(false, false, true, false);
private String[] types = Strings.EMPTY_ARRAY;
private String routing;
private TimeValue timeout;

Expand Down Expand Up @@ -96,37 +95,15 @@ public GraphExploreRequest indicesOptions(IndicesOptions indicesOptions) {
return this;
}

/**
* The document types to execute the explore against. Defaults to be executed against
* all types.
*
* @deprecated Types are in the process of being removed. Instead of using a type, prefer to
* filter on a field on the document.
*/
@Deprecated
public String[] types() {
return this.types;
}

/**
* The document types to execute the explore request against. Defaults to be executed against
* all types.
*
* @deprecated Types are in the process of being removed. Instead of using a type, prefer to
* filter on a field on the document.
*/
@Deprecated
public GraphExploreRequest types(String... types) {
this.types = types;
return this;
}

public GraphExploreRequest(StreamInput in) throws IOException {
super(in);

indices = in.readStringArray();
indicesOptions = IndicesOptions.readIndicesOptions(in);
types = in.readStringArray();
if (in.getVersion().before(Version.V_8_0_0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned this in the PR for the explain API as well -- I think it'd be nice to read the types array and assert that it is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++

String[] types = in.readStringArray();
assert types.length == 0;
}
routing = in.readOptionalString();
timeout = in.readOptionalTimeValue();
sampleSize = in.readInt();
Expand Down Expand Up @@ -169,7 +146,7 @@ public TimeValue timeout() {
* operations involved in each hop are limited to the remaining time
* available but can still overrun due to the nature of their "best efforts"
* timeout support. When a timeout occurs partial results are returned.
*
*
* @param timeout
* a {@link TimeValue} object which determines the maximum length
* of time to spend exploring
Expand All @@ -192,7 +169,9 @@ public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeStringArray(indices);
indicesOptions.writeIndicesOptions(out);
out.writeStringArray(types);
if (out.getVersion().before(Version.V_8_0_0)) {
out.writeStringArray(Strings.EMPTY_ARRAY);
}
out.writeOptionalString(routing);
out.writeOptionalTimeValue(timeout);

Expand All @@ -203,15 +182,14 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeBoolean(useSignificance);
out.writeBoolean(returnDetailedInfo);
out.writeInt(hops.size());
for (Iterator<Hop> iterator = hops.iterator(); iterator.hasNext();) {
Hop hop = iterator.next();
for (Hop hop : hops) {
hop.writeTo(out);
}
}

@Override
public String toString() {
return "graph explore [" + Arrays.toString(indices) + "][" + Arrays.toString(types) + "]";
return "graph explore [" + Arrays.toString(indices) + "]";
}

/**
Expand All @@ -227,7 +205,7 @@ public String toString() {
* better with smaller samples as there are less look-ups required for
* background frequencies of terms found in the documents
* </p>
*
*
* @param maxNumberOfDocsPerHop
* shard-level sample size in documents
*/
Expand Down Expand Up @@ -268,7 +246,7 @@ public int maxDocsPerDiversityValue() {
* default value is true which means terms are selected based on
* significance (see the {@link SignificantTerms} aggregation) rather than
* popularity (using the {@link TermsAggregator}).
*
*
* @param value
* true if the significant_terms algorithm should be used.
*/
Expand All @@ -283,7 +261,7 @@ public boolean useSignificance() {
/**
* Return detailed information about vertex frequencies as part of JSON
* results - defaults to false
*
*
* @param value
* true if detailed information is required in JSON responses
*/
Expand All @@ -299,7 +277,7 @@ public boolean returnDetailedInfo() {
* Add a stage in the graph exploration. Each hop represents a stage of
* querying elasticsearch to identify terms which can then be connnected to
* other terms in a subsequent hop.
*
*
* @param guidingQuery
* optional choice of query which influences which documents are
* considered in this stage
Expand Down Expand Up @@ -364,7 +342,7 @@ void writeTo(StreamOutput out) throws IOException {
@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();

builder.startObject("controls");
{
if (sampleSize != SamplerAggregationBuilder.DEFAULT_SHARD_SAMPLE_SIZE) {
Expand Down
29 changes: 10 additions & 19 deletions ...e/src/main/java/org/elasticsearch/xpack/core/graph/action/GraphExploreRequestBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

/**
* Creates a new {@link GraphExploreRequestBuilder}
*
*
* @see GraphExploreRequest
*/
public class GraphExploreRequestBuilder extends ActionRequestBuilder<GraphExploreRequest, GraphExploreResponse> {
Expand Down Expand Up @@ -106,16 +106,7 @@ public GraphExploreRequestBuilder setTimeout(String timeout) {
}

/**
* The types of documents the graph exploration will run against. Defaults
* to all types.
*/
public GraphExploreRequestBuilder setTypes(String... types) {
request.types(types);
return this;
}

/**
* Add a stage in the graph exploration. Each hop represents a stage of
* Add a stage in the graph exploration. Each hop represents a stage of
* querying elasticsearch to identify terms which can then be connnected
* to other terms in a subsequent hop.
* @param guidingQuery optional choice of query which influences which documents
Expand All @@ -129,27 +120,27 @@ public Hop createNextHop(@Nullable QueryBuilder guidingQuery) {
/**
* Controls the choice of algorithm used to select interesting terms. The default
* value is true which means terms are selected based on significance (see the {@link SignificantTerms}
* aggregation) rather than popularity (using the {@link TermsAggregator}).
* aggregation) rather than popularity (using the {@link TermsAggregator}).
* @param value true if the significant_terms algorithm should be used.
*/
public GraphExploreRequestBuilder useSignificance(boolean value) {
request.useSignificance(value);
return this;
}


/**
* The number of top-matching documents that are considered during each hop (default is
* The number of top-matching documents that are considered during each hop (default is
* {@link SamplerAggregationBuilder#DEFAULT_SHARD_SAMPLE_SIZE}
* Very small values (less than 50) may not provide sufficient weight-of-evidence to identify
* significant connections between terms.
* <p> Very large values (many thousands) are not recommended with loosely defined queries (fuzzy queries or
* significant connections between terms.
* <p> Very large values (many thousands) are not recommended with loosely defined queries (fuzzy queries or
* those with many OR clauses).
* This is because any useful signals in the best documents are diluted with irrelevant noise from low-quality matches.
* Performance is also typically better with smaller samples as there are less look-ups required for background frequencies
* of terms found in the documents
* Performance is also typically better with smaller samples as there are less look-ups required for background frequencies
* of terms found in the documents
* </p>
*
*
* @param maxNumberOfDocsPerHop the shard-level sample size in documents
*/
public GraphExploreRequestBuilder sampleSize(int maxNumberOfDocsPerHop) {
Expand Down
22 changes: 4 additions & 18 deletions ...plugin/graph/src/main/java/org/elasticsearch/xpack/graph/rest/action/RestGraphAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@
public class RestGraphAction extends BaseRestHandler {

private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(RestGraphAction.class));
public static final String TYPES_DEPRECATION_MESSAGE = "[types removal]" +
" Specifying types in graph requests is deprecated.";

public static final ParseField TIMEOUT_FIELD = new ParseField("timeout");
public static final ParseField SIGNIFICANCE_FIELD = new ParseField("use_significance");
Expand All @@ -65,20 +63,12 @@ public class RestGraphAction extends BaseRestHandler {
public RestGraphAction(RestController controller) {
// TODO: remove deprecated endpoint in 8.0.0
controller.registerWithDeprecatedHandler(
GET, "/{index}/_graph/explore", this,
GET, "/{index}/_xpack/graph/_explore", deprecationLogger);
GET, "/{index}/_graph/explore", this,
GET, "/{index}/_xpack/graph/_explore", deprecationLogger);
// TODO: remove deprecated endpoint in 8.0.0
controller.registerWithDeprecatedHandler(
POST, "/{index}/_graph/explore", this,
POST, "/{index}/_xpack/graph/_explore", deprecationLogger);
// TODO: remove deprecated endpoint in 8.0.0
controller.registerWithDeprecatedHandler(
GET, "/{index}/{type}/_graph/explore", this,
GET, "/{index}/{type}/_xpack/graph/_explore", deprecationLogger);
// TODO: remove deprecated endpoint in 8.0.0
controller.registerWithDeprecatedHandler(
POST, "/{index}/{type}/_graph/explore", this,
POST, "/{index}/{type}/_xpack/graph/_explore", deprecationLogger);
POST, "/{index}/_graph/explore", this,
POST, "/{index}/_xpack/graph/_explore", deprecationLogger);
}

@Override
Expand Down Expand Up @@ -111,10 +101,6 @@ public RestChannelConsumer prepareRequest(final RestRequest request, final NodeC
parseHop(parser, currentHop, graphRequest);
}

if (request.hasParam("type")) {
deprecationLogger.deprecatedAndMaybeLog("graph_with_types", TYPES_DEPRECATION_MESSAGE);
graphRequest.types(Strings.splitStringByCommaToArray(request.param("type")));
}
return channel -> client.execute(INSTANCE, graphRequest, new RestToXContentListener<>(channel));
}

Expand Down
Loading