Add rest highlevel explain API #31387

PnPie · 2018-06-17T13:38:58Z

Add explain API to rest high level client.

I create this PR firstly and I always have problem while parsing GetReult of ExplainResponse through XContent. I used GetResult.toXContentEmbedded/fromXContentEmbedded and it works well in GetResultTests, but in ExplainResponseTests seems it always fails with some arbitrary fields added in it. Anyone has any idea ?

Relates to #27205

Add explain API to rest high level client.

elasticmachine · 2018-06-18T09:16:00Z

Pinging @elastic/es-core-infra

javanna

I left some comments, thanks for your PR @PnPie !

javanna · 2018-06-18T09:24:38Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java

+        Params params = new Params(request);
+        params.withStoredFields(explainRequest.storedFields());
+        params.withFetchSourceContext(explainRequest.fetchSourceContext());
+        params.withRouting(explainRequest.routing());


aren't we missing the parent parameter?

The parent parameter is to set the routing in fact, so here we can just use withRouting to set it.

Correct, we have removed support for parent in master. I will have to remember this when backporting to 6.x where we still support parent.

scratch that, I think this will be fine as-is in 6.x as well.

javanna · 2018-06-18T10:02:12Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

@@ -826,6 +855,14 @@ protected final ElasticsearchStatusException parseResponseException(ResponseExce
        }
    }

+    private <Resp> Resp processResponse(Resp resp, int statusCode) {


I see the problem here: the response doesn't print out the exists flag, which should be assigned based on the status code. I would not check instanceof though here, I think it's possible to provide a context to the fromXContent method as an additional argument (currently declared Void so it is always null), make that a Boolean . It is not very easy though to make that work based on the status in the client. As an alternative, we could change the output of the API and make it print out the flag as well, like many other API do. @nik9000 what do you think?

Could you make the responseConverter something like

response -> { ExplianResponse r = parseEntity(response.getEntity(), ExplainResponse::fromXContent); r.setExists(convertExistsResponse(response)); return r; }

?

Ah! I see, you want the boolean as the context so you don't need to add setExists. You could do that like this:

response -> { CheckedFunction<XContentParser, Resp, IOException> entityParser = parser -> ExplainResponse.fromXContent(convertExistsResponse(response), parser); return parseEntity(response.getEntity(), entityParser); }

You make ExplainResponse.fromXContent like this instead of how you have it.

public static ExplainResponse fromXContent(boolean exists, XContentParser parser) { return PARSER.apply(parser, exists); }

I think that isn't too bad.

And you'd have to change the parser declaration to:

private static final ConstructingObjectParser<ExplainResponse, Boolean> PARSER = new ConstructingObjectParser<>("explain", true, (args, exists) -> new ExplainResponse((String) args[0], (String) args[1], (String) args[2], exists, (Explanation) args[3], + (GetResult) arg[4]));

Or something like that. I'm not 100% sure on that bit to be honest, but somewhere in the building function you'd use exists.
And then you can

I see it ! thx

javanna · 2018-06-18T10:04:20Z

server/src/main/java/org/elasticsearch/action/explain/ExplainResponse.java

+        PARSER.declareString(ConstructingObjectParser.constructorArg(), _INDEX);
+        PARSER.declareString(ConstructingObjectParser.constructorArg(), _TYPE);
+        PARSER.declareString(ConstructingObjectParser.constructorArg(), _ID);
+        PARSER.declareBoolean(ConstructingObjectParser.constructorArg(), MATCHED);


I think that you end up passing matched in as exists in the ExplainResponse constructor.

I removed this as the exists field is not necessary to be parsed through XContent using the way suggested by @nik9000

javanna · 2018-06-18T10:05:20Z

server/src/main/java/org/elasticsearch/action/explain/ExplainResponse.java


 import static org.elasticsearch.common.lucene.Lucene.readExplanation;
 import static org.elasticsearch.common.lucene.Lucene.writeExplanation;

 /**
 * Response containing the score explanation.
 */
-public class ExplainResponse extends ActionResponse {
+public class ExplainResponse extends ActionResponse implements ToXContentObject {


you could make this StatusToXContentObject and have status method return the proper status based on the exists flag

javanna · 2018-06-18T10:38:58Z

server/src/main/java/org/elasticsearch/action/explain/ExplainResponse.java

+        explanationParser.declareString(ConstructingObjectParser.constructorArg(), DESCRIPTION);
+        explanationParser.declareObjectArray(ConstructingObjectParser.constructorArg(), explanationParser, DETAILS);
+        PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), explanationParser, EXPLANATION);
+        PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), (p, c) -> GetResult.fromXContentEmbedded(p), GET);


does this parse the explanation recursively like it's printed out in toXcontent?

Yes, I put the explanationParser recursively here https://github.com/elastic/elasticsearch/pull/31387/files#diff-86e50d6115fc7c8e90bfabdbb354886cR174 and the Explanation parsing works well as I tested in ExplainResponseTests.

javanna · 2018-06-18T10:39:30Z

server/src/main/java/org/elasticsearch/action/explain/ExplainResponse.java

+    }
+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {


I think that you can remove this code from RestExplainAction and call this one instead by using RestStatusToXContentListener

javanna · 2018-06-18T10:42:25Z

server/src/test/java/org/elasticsearch/action/explain/ExplainResponseTests.java

+import static java.util.Collections.singletonMap;
+import static org.hamcrest.Matchers.equalTo;
+
+public class ExplainResponseTests extends AbstractStreamableXContentTestCase<ExplainResponse> {


I think it is expected that injecting random fields causes problems in some parts of the response, for instance the section where the source is returned. I guess that some path needs to be excluded using getRandomFieldsExcludeFilter

PnPie

Thanks for your review and comments @javanna @nik9000 ! I updated the PR.

PnPie · 2018-06-20T03:04:30Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RequestConverters.java

+        Params params = new Params(request);
+        params.withStoredFields(explainRequest.storedFields());
+        params.withFetchSourceContext(explainRequest.fetchSourceContext());
+        params.withRouting(explainRequest.routing());


The parent parameter is to set the routing in fact, so here we can just use withRouting to set it.

PnPie · 2018-06-20T03:05:29Z

client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java

@@ -826,6 +855,14 @@ protected final ElasticsearchStatusException parseResponseException(ResponseExce
        }
    }

+    private <Resp> Resp processResponse(Resp resp, int statusCode) {


I see it ! thx

PnPie · 2018-06-20T03:13:17Z

server/src/main/java/org/elasticsearch/action/explain/ExplainResponse.java

+        PARSER.declareString(ConstructingObjectParser.constructorArg(), _INDEX);
+        PARSER.declareString(ConstructingObjectParser.constructorArg(), _TYPE);
+        PARSER.declareString(ConstructingObjectParser.constructorArg(), _ID);
+        PARSER.declareBoolean(ConstructingObjectParser.constructorArg(), MATCHED);


I removed this as the exists field is not necessary to be parsed through XContent using the way suggested by @nik9000

PnPie · 2018-06-20T03:17:08Z

server/src/main/java/org/elasticsearch/action/explain/ExplainResponse.java

+        explanationParser.declareString(ConstructingObjectParser.constructorArg(), DESCRIPTION);
+        explanationParser.declareObjectArray(ConstructingObjectParser.constructorArg(), explanationParser, DETAILS);
+        PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), explanationParser, EXPLANATION);
+        PARSER.declareObject(ConstructingObjectParser.optionalConstructorArg(), (p, c) -> GetResult.fromXContentEmbedded(p), GET);


Yes, I put the explanationParser recursively here https://github.com/elastic/elasticsearch/pull/31387/files#diff-86e50d6115fc7c8e90bfabdbb354886cR174 and the Explanation parsing works well as I tested in ExplainResponseTests.

javanna · 2018-06-21T12:40:54Z

test this please

javanna

I left a couple of comments, this looks good though, thanks @PnPie !

javanna · 2018-06-21T12:39:24Z

server/src/test/java/org/elasticsearch/action/explain/ExplainResponseTests.java

+
+    @Override
+    protected Predicate<String> getRandomFieldsExcludeFilter() {
+        return field -> field.startsWith("fields") || field.startsWith("get");


I think that field.startsWith("get") could become field.startsWith("get._source") || field.startsWith("get.fields")

Hi @javanna I change this line to field.startsWith("get._source") || field.startsWith("get.fields") but it still adds random items to "get.fields", it seems in this case it randomly inserts when field is "get" ?
Because when I do this field.startsWith("get._source") || field.startsWith("get.fields") || field.equals("get") then it works well.
So I keep this as field.startsWith("get") ?

good point, that is because GetResult prints out fields also at the get level, depending on whether they are metadata fields or not. So any random field at the get level will be parsed back as a field and printed out as part of fields.

You can use field.equals("get") || field.startsWith("get.fields") || field.startsWith("get._source")

javanna · 2018-06-21T12:40:17Z

server/src/test/java/org/elasticsearch/action/explain/ExplainResponseTests.java

+        assertThat(expectedResponse, equalTo(generatedResponse));
+    }
+
+    private ExplainResponse createSimpleResponse() {


given that we use this in a single place, can we remove this method and put the code where it's currently called?

javanna · 2018-06-21T12:40:23Z

server/src/test/java/org/elasticsearch/action/explain/ExplainResponseTests.java

+        return new ExplainResponse(index, type, id, exist, explanation, getResult);
+    }
+
+    private Explanation randomExplanation(Explanation... explanations) {


can this be static?

javanna · 2018-06-26T12:56:39Z

docs/java-rest/high-level/search/explain.asciidoc

+--------------------------------------------------
+
+[[java-rest-high-explain-request-optional]]
+===== Optional arguments


I think that this should be at the same level as request. I noticed some other pages where we do it this way but I prefer to have them at the same level.

javanna · 2018-06-26T12:59:02Z

client/rest-high-level/src/test/java/org/elasticsearch/client/SearchIT.java

@@ -835,6 +877,174 @@ public void testRenderSearchTemplate() throws IOException {
        assertToXContentEquivalent(expectedSource, actualSource, XContentType.JSON);
    }

+    public void testExplainSimple() throws IOException {


I would prefer not using Simple in a method name, although it's a test. Nothing is really simple :) Call it testExplain?

Yeah definitly agree xD

javanna

I left a couple more comments and replied to your latest one. Thanks for doing this @PnPie I will merge it as soon as the comments are addressed.

javanna · 2018-06-26T13:00:08Z

test this please

PnPie · 2018-06-27T07:29:29Z

Thx @javanna I updated it.

javanna · 2018-06-27T07:53:23Z

retest this please

javanna · 2018-06-27T11:32:28Z

...t-high-level/src/test/java/org/elasticsearch/client/documentation/SearchDocumentationIT.java

@@ -1046,7 +1131,7 @@ private void indexSearchTestData() throws IOException {
        assertTrue(authorsResponse.isAcknowledged());

        CreateIndexRequest reviewersRequest = new CreateIndexRequest("contributors")
-            .mapping("doc", "user", "type=keyword");
+            .mapping("doc", "user", "type=keyword,store=true");


was this change necessary?

I have tested the "stored_fields" flag in testExplain() and I add an assertion here, if we don't "store" this field we will not get any values in GetResult's fields, so this assertion will not work. Otherwise we remove this assertion ?

got it, it's fine, I got confused by the github diff, it seemed like this was part of testFieldCaps only, but it's shared between different methods. All good!

javanna · 2018-06-27T13:21:19Z

thanks @PnPie !

nik9000 · 2018-06-27T18:09:07Z

@javanna I bumped into a failure locally after merging this that reproduced so I pushed ce1d0f5 to fix it. Could you bundle that when you backport this?

* elastic/master: (57 commits) HLRest: Fix test for explain API [TEST] Fix RemoteClusterConnectionTests Add Create Snapshot to High-Level Rest Client (elastic#31215) Remove legacy MetaDataStateFormat (elastic#31603) Add explain API to high-level REST client (elastic#31387) Preserve thread context when connecting to remote cluster (elastic#31574) Unify headers for full text queries Remove redundant 'minimum_should_match' JDBC driver prepared statement set* methods (elastic#31494) [TEST] call yaml client close method from test suite (elastic#31591) ingest: Add ignore_missing property to foreach filter (elastic#22147) (elastic#31578) Fix a formatting issue in the docvalue_fields documentation. (elastic#31563) reduce log level at gradle configuration time [TEST] Close additional clients created while running yaml tests (elastic#31575) Docs: Clarify sensitive fields watcher encryption (elastic#31551) Watcher: Remove never executed code (elastic#31135) Add support for switching distribution for all integration tests (elastic#30874) Improve robustness of geo shape parser for malformed shapes (elastic#31449) QA: Create xpack yaml features (elastic#31403) Improve test times for tests using `RandomObjects::addFields` (elastic#31556) ...

javanna · 2018-06-27T19:16:48Z

@nik9000 thanks for catching and fixing, I will integrate your fix when backporting.

Relates to #27205

* master: Docs: Remove duplicate test setup Print output when the name checker IT fails (#31660) Fix syntax errors in get-snapshots docs (#31656) Docs: Fix description of percentile ranks example example (#31652) Add MultiSearchTemplate support to High Level Rest client (#30836) Add test for low-level client round-robin behaviour (#31616) SQL: Refactor package names of sql-proto and sql-shared-proto projects (#31622) Remove deprecation warnings to prepare for Gradle 5 (sourceSets.main.output.classesDirs) (#30389) Correct integTest enable logic (#31646) Fix missing get-snapshots docs reference #31645 Do not check for Azure container existence (#31617) Merge AwsS3Service and InternalAwsS3Service in a S3Service class (#31580) Upgrade gradle wrapper to 4.8 (#31525) Only set vm.max_map_count if greater than default (#31512) Add Get Snapshots High Level REST API (#31537) QA: Merge query-builder-bwc to restart test (#30979) Update reindex.asciidoc (#31626) Docs: Skip xpack snippet tests if no xpack (#31619) mute CreateSnapshotRequestTests HLRest: Fix test for explain API [TEST] Fix RemoteClusterConnectionTests Add Create Snapshot to High-Level Rest Client (#31215) Remove legacy MetaDataStateFormat (#31603) Add explain API to high-level REST client (#31387) Preserve thread context when connecting to remote cluster (#31574) Unify headers for full text queries Remove redundant 'minimum_should_match' JDBC driver prepared statement set* methods (#31494) [TEST] call yaml client close method from test suite (#31591)

* 6.x: Do not check for object existence when deleting repository index files (#31680) Remove extra check for object existence in repository-gcs read object (#31661) [Test] Clean up some repository-s3 tests (#31601) Merge AzureStorageService and AzureStorageServiceImpl and clean up tests (#31607) [Docs] Use capital letters in section headings (#31678) [DOCS] Add PQL language Plugin (#31237) [DOCS] Fix licensing API details (#31667) Fix CreateSnapshotRequestTests Failure (#31630) Add Create Snapshot to High-Level Rest Client (#31215) Add MultiSearchTemplate support to High Level Rest client (#31662) SQL: Refactor package names of sql-proto and sql-shared-proto projects (#31622) [TEST] Mute failing NamingConventionsTaskIT tests [DOCS] Replace CONFIG_DIR with ES_PATH_CONF (#31635) Add test for low-level client round-robin behaviour (#31616) HLRest: Fix test for explain API Add explain API to high-level REST client (#31387)

Rest highlevel explain API

1616e41

Add explain API to rest high level client.

javanna added the :Core/Java High Level REST Client label Jun 18, 2018

javanna added >enhancement v7.0.0 v6.4.0 labels Jun 18, 2018

javanna self-requested a review June 18, 2018 09:16

javanna mentioned this pull request Jun 18, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

javanna reviewed Jun 18, 2018

View reviewed changes

address comments

35b9535

PnPie commented Jun 20, 2018

View reviewed changes

javanna approved these changes Jun 21, 2018

View reviewed changes

test changes & remove unused imports

dce46fb

javanna reviewed Jun 26, 2018

View reviewed changes

PnPie added 2 commits June 27, 2018 15:20

address comments & remove unused imports

466b79e

Merge branch 'master' into rest_highlevel_explain

71ca72c

javanna reviewed Jun 27, 2018

View reviewed changes

javanna merged commit 616703b into elastic:master Jun 27, 2018

javanna added the backport pending label Jun 27, 2018

javanna pushed a commit that referenced this pull request Jun 28, 2018

Add explain API to high-level REST client (#31387)

ea2f68e

Relates to #27205

javanna removed the backport pending label Jun 28, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Add rest highlevel explain API #31387

Add rest highlevel explain API #31387

Conversation

PnPie commented Jun 17, 2018 • edited Loading

elasticmachine commented Jun 18, 2018

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PnPie Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PnPie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PnPie Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

javanna commented Jun 21, 2018

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna left a comment

Choose a reason for hiding this comment

javanna commented Jun 26, 2018

PnPie commented Jun 27, 2018

javanna commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Jun 27, 2018

nik9000 commented Jun 27, 2018

javanna commented Jun 27, 2018

PnPie commented Jun 17, 2018 •

edited

Loading

PnPie Jun 20, 2018 •

edited

Loading

PnPie Jun 20, 2018 •

edited

Loading