HLRC API for _termvectors by mayya-sharipova · Pull Request #33447 · elastic/elasticsearch

mayya-sharipova · 2018-09-05T22:08:00Z

relates to #27205

elasticmachine · 2018-09-05T22:08:19Z

Pinging @elastic/es-core-infra

nik9000 · 2018-09-19T15:36:35Z

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

+        if (docBuilder != null) {
+            BytesReference doc = BytesReference.bytes(docBuilder);
+            try (InputStream stream = doc.streamInput()) {
+                builder.rawField("doc", stream, docBuilder.contentType());


I really don't like the name of this method! It makes me think we're copying the field raw without checking the content type but we are checking it.

nik9000 · 2018-09-19T15:41:48Z

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

+    private final long docVersion;
+    private final boolean found;
+    private final long tookInMillis;
+    private List<TermVector> termVectorList = null;


It feels a little weird to have just this part be mutable.

is it possible for the ConstructingObjectParser to pass in a NamedObjects constructorArg? Right now on line 67 is where its being mutated.

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

nik9000 · 2018-09-19T15:45:40Z

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

+            public Term() {};
+
+            public static Term fromXContent(XContentParser parser, String term) {
+                Term t = TERMPARSER.apply(parser, null);


Maybe put TERMPARSER into this class?

The same for the other parsers, I think.

nik9000 · 2018-09-19T15:46:41Z

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

+            private float score = -1f;
+            private List<Token> tokens = null;
+
+            public Term() {};


Why is this one mutable when the others are immutable? Maybe make all immutable?

hub-cap

Awesome work thus far. Had some comments / nits to address. It looks mostly done tho.

hub-cap · 2018-09-19T17:09:26Z

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


+    static Request termVectors(TermVectorsRequest tvrequest) throws IOException {
+        String endpoint = "_termvectors";
+        if (tvrequest.getId() != null) {


the EndpointBuilder() gladly accepts nulls in its addPathPart(...) method. You dont need to worry about passing a null value in, as it just wont put it in the path.

hub-cap · 2018-09-19T17:12:14Z

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

+        }
+        Request request = new Request(HttpGet.METHOD_NAME, endpoint);
+        Params params = new Params(request);
+        if (tvrequest.getRouting() != null) params.withRouting(tvrequest.getRouting());


the params methods all call putParam and that does a null check, and will not add it if its null. So no need to check here.

hub-cap · 2018-09-19T17:25:32Z

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

+
+        if (filterSettings != null) {
+            builder.startObject("filter");
+            if (filterSettings.containsKey("max_num_terms")) builder.field("max_num_terms", filterSettings.get("max_num_terms"));


maybe a helper method here to reduce the copy/paste

hub-cap · 2018-09-19T17:27:59Z

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

+    private final long docVersion;
+    private final boolean found;
+    private final long tookInMillis;
+    private List<TermVector> termVectorList = null;


is it possible for the ConstructingObjectParser to pass in a NamedObjects constructorArg? Right now on line 67 is where its being mutated.

hub-cap · 2018-09-19T17:28:15Z

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

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


I dont think we need this

hub-cap · 2018-09-19T17:39:01Z

...nt/rest-high-level/src/test/java/org/elasticsearch/client/core/TermVectorsResponseTests.java

+import java.util.function.Predicate;
+
+
+public class TermVectorsResponseTests extends AbstractXContentTestCase<TermVectorsResponse> {


when removing the ToXContent, you will have to manually create a parser to test. This could be nasty on such a large class hierarchy. Im sorry, but its the best we have now. We need to come up with a better way. You should be able to move the code out of the response and just use it in a test.

hub-cap · 2018-09-19T17:52:47Z

server/src/main/java/org/elasticsearch/index/reindex/DeleteByQueryRequest.java

    }

-    DeleteByQueryRequest(SearchRequest search) {
+    public DeleteByQueryRequest(SearchRequest search) {


Is this intended? I dont see any uses of it here.

Probably drop this, yeah.

- use ConstructorObjectParser to make object fields final

…rm-vectors-new-response-format

- move toXContent section from TermVectorsResponse to TermVectorsResponseTests - refactors docs to use concise version

mayya-sharipova · 2018-10-17T20:44:00Z

@hub-cap @nik9000 I have tried to address your comments, can you please continue the review.

nik9000

I left a bunch of minor stuff but I think it is pretty much done!

nik9000 · 2018-10-17T21:09:35Z

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

+        this.id = docId;
+    }
+
+    public String getIndex() {


Could you put these below the other ctor?

And maybe add javadoc for them like you did with the other methods?

nik9000 · 2018-10-17T21:12:02Z

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

+            String[] filterSettingNames =
+                {"max_num_terms", "min_term_freq", "max_term_freq", "min_doc_freq", "max_doc_freq", "min_word_length", "max_word_length"};
+            for (String settingName : filterSettingNames) {
+                if (filterSettings.containsKey(settingName)) builder.field(settingName, filterSettings.get(settingName));


Should these be a class rather than a free form map? It looks like this'd silently not serialize some stuff and that seems bad.

All these parameters are optional, and a request doesn't require any of them, that is why I think it is fine to have it as a map.

nik9000 · 2018-10-17T21:12:50Z

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

+    private final List<TermVector> termVectorList;
+
+    public TermVectorsResponse(
+        String index, String type, String id, long version, boolean found, long tookInMillis, List<TermVector> termVectorList) {


Could you indent this one more time so the parameters don't line up with the body?

nik9000 · 2018-10-17T21:13:43Z

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

+        this.found = found;
+        this.tookInMillis = tookInMillis;
+        if (termVectorList != null) {
+            Collections.sort(termVectorList, Comparator.comparing(TermVector::getFieldName));


I'd probably sort in the parser rather than the ctor. It just feels a little less surprising.

nik9000 · 2018-10-17T21:14:55Z

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

+
+    @SuppressWarnings("unchecked")
+    private static ConstructingObjectParser<TermVectorsResponse, Void> PARSER = new ConstructingObjectParser<>(
+        "term_vectors", true, args  -> new TermVectorsResponse((String) args[0], (String) args[1],


I'd probably make this a block with a return statement because that'd make it a bit easier to read. I think.

nik9000 · 2018-10-17T21:18:46Z

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

+                return totalTermFreq;
+            }
+
+            public Float getScore(){


Moar javadoc!

nik9000 · 2018-10-17T21:19:21Z

...nt/rest-high-level/src/test/java/org/elasticsearch/client/core/TermVectorsResponseTests.java

+    }
+
+    private void toXContent(TermVectorsResponse response, XContentBuilder builder) throws IOException {
+       builder.startObject();


funny indentation

nik9000 · 2018-10-17T21:20:44Z

...est-high-level/src/test/java/org/elasticsearch/client/documentation/CRUDDocumentationIT.java

        }
    }

+    public void testTermVectors() throws Exception {


I don't think getting the term vectors is part of CRUD. I don't know what it is, but I don't think it is CRUD. I guess if there isn't any better place, here is good. But maybe leave a comment?

nik9000 · 2018-10-17T21:22:35Z

...est-high-level/src/test/java/org/elasticsearch/client/documentation/CRUDDocumentationIT.java

+                                int position = token.getPosition(); // <18>
+                                int startOffset = token.getStartOffset(); // <19>
+                                int endOffset = token.getEndOffset(); // <20>
+                                String payload = token.getPayload(); // <21>


Wow that is a lot of callouts! Maybe it'd be easier to read the docs if you split it? I'm not sure how that'd look, but 21 callouts is going to be super hard to read!

nik9000 · 2018-10-17T21:23:01Z

server/src/main/java/org/elasticsearch/index/reindex/DeleteByQueryRequest.java

    }

-    DeleteByQueryRequest(SearchRequest search) {
+    public DeleteByQueryRequest(SearchRequest search) {


Probably drop this, yeah.

…rm-vectors-new-response-format

mayya-sharipova · 2018-10-20T14:02:34Z

@nik9000 Thanks for such a detailed feedback, Nik. I have addressed all your comments, can you please continue the review.

The only comment I have not addressed is to make filterSettings a class in the TermVectorsRequest. This is because as all the parameters in filterSettings are optional, and a request doesn't require any of them, I think it is fine to have filterSettings as a map.

hub-cap

one super minor nit. also, wow you picked a giant client call!! Thank you for taking the time to put the new classes into the core folder in client instead of using the existing server ones. Itll be one less thing to do to decouple. <3 <3

hub-cap · 2018-10-22T19:32:28Z

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

+     * @param docId - id of the document
+     */
+    public TermVectorsRequest(String index, String type, String docId) {
+        this.index = index;


this(index, type); this.id = docId;

Sort lists for equality in parsers

…rm-vectors-new-response-format

mayya-sharipova · 2018-10-23T18:53:15Z

@hub-cap thank you for the review.

@nik9000 Do you still want to make the final round of review, or I can merge this PR?

nik9000

LGTM

relates to #33447

relates to elastic#33447

relates to #33447

* HLRC API for _termvectors relates to #27205

relates to #33447

Relates to elastic#33447

Relates to #33447

mayya-sharipova added the :Core/Java High Level REST Client label Sep 5, 2018

mayya-sharipova added >enhancement v7.0.0 labels Sep 5, 2018

mayya-sharipova mentioned this pull request Sep 5, 2018

HLRC API for _termvectors #32610

Closed

javanna mentioned this pull request Sep 6, 2018

Java high-level REST client completeness #27205

Closed

80 tasks

HLRC API for _termvectors

2272642

relates to elastic#27205

mayya-sharipova force-pushed the rest-highlevel-term-vectors-new-response-format branch from 76c90c5 to 2272642 Compare September 6, 2018 15:51

mayya-sharipova added the review label Sep 6, 2018

mayya-sharipova requested a review from hub-cap September 6, 2018 15:53

nik9000 reviewed Sep 19, 2018

View reviewed changes

hub-cap suggested changes Sep 19, 2018

View reviewed changes

Address comments

75a0c63

- use ConstructorObjectParser to make object fields final

rjernst removed the review label Oct 10, 2018

mayya-sharipova added 2 commits October 16, 2018 16:04

Merge remote-tracking branch 'upstream/master' into rest-highlevel-te…

e09d885

…rm-vectors-new-response-format

Address comments

7056664

- move toXContent section from TermVectorsResponse to TermVectorsResponseTests - refactors docs to use concise version

nik9000 reviewed Oct 17, 2018

View reviewed changes

mayya-sharipova added 2 commits October 19, 2018 16:59

Address Nik's feedback

282e04e

Merge remote-tracking branch 'upstream/master' into rest-highlevel-te…

227b6bd

…rm-vectors-new-response-format

hub-cap approved these changes Oct 22, 2018

View reviewed changes

mayya-sharipova added 2 commits October 23, 2018 10:18

Address comments

e31df93

Sort lists for equality in parsers

Merge remote-tracking branch 'upstream/master' into rest-highlevel-te…

8a1998c

…rm-vectors-new-response-format

nik9000 approved these changes Oct 24, 2018

View reviewed changes

mayya-sharipova merged commit bf4d90a into elastic:master Oct 24, 2018

mayya-sharipova deleted the rest-highlevel-term-vectors-new-response-format branch October 24, 2018 18:27

mayya-sharipova added a commit that referenced this pull request Oct 24, 2018

Correct CRUDDocumentationIT.java

6cceba9

relates to #33447

mayya-sharipova added a commit that referenced this pull request Oct 24, 2018

Correct term-vectors.asciidoc

3211760

relates to #33447

mayya-sharipova added a commit that referenced this pull request Oct 24, 2018

Reduce the number of callouts to 15 in docs

d7afd7c

relates to #33447

tvernum mentioned this pull request Oct 25, 2018

[Test] HLRC: TermVectorsResponseTests fails with some seeds #34837

Closed

mayya-sharipova added v6.6.0 backport pending labels Oct 26, 2018

mayya-sharipova added a commit to mayya-sharipova/elasticsearch that referenced this pull request Oct 26, 2018

HLRC API for _termvectors

57ba70b

relates to elastic#33447

mayya-sharipova mentioned this pull request Oct 26, 2018

HLRC API for _termvectors #34933

Merged

mayya-sharipova added a commit that referenced this pull request Oct 27, 2018

HLRC API for _termvectors (#34933)

c4325a5

relates to #33447

mayya-sharipova removed the backport pending label Oct 27, 2018

kcm pushed a commit that referenced this pull request Oct 30, 2018

HLRC API for _termvectors (#33447)

bdeb42a

* HLRC API for _termvectors relates to #27205

kcm pushed a commit that referenced this pull request Oct 30, 2018

Correct CRUDDocumentationIT.java

241cf40

relates to #33447

kcm pushed a commit that referenced this pull request Oct 30, 2018

Correct term-vectors.asciidoc

44db922

relates to #33447

kcm pushed a commit that referenced this pull request Oct 30, 2018

Reduce the number of callouts to 15 in docs

a0b706d

relates to #33447

mayya-sharipova added a commit to mayya-sharipova/elasticsearch that referenced this pull request Nov 2, 2018

Small corrections to HLRC doc for _termvectors

4c7fcf2

Relates to elastic#33447

mayya-sharipova mentioned this pull request Nov 2, 2018

Small corrections to HLRC doc for _termvectors #35221

Merged

mayya-sharipova added a commit that referenced this pull request Nov 4, 2018

Small corrections to HLRC doc for _termvectors (#35221)

a2c22ad

Relates to #33447

mayya-sharipova added a commit that referenced this pull request Nov 5, 2018

Small corrections to HLRC doc for _termvectors (#35221)

54365fc

Relates to #33447

mayya-sharipova mentioned this pull request Nov 5, 2018

HLRC for _mtermvectors #35266

Merged

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

		import java.util.function.Predicate;


		public class TermVectorsResponseTests extends AbstractXContentTestCase<TermVectorsResponse> {

Conversation

mayya-sharipova commented Sep 5, 2018

Uh oh!

elasticmachine commented Sep 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayya-sharipova commented Oct 17, 2018

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayya-sharipova commented Oct 20, 2018

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mayya-sharipova commented Oct 23, 2018

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees