Refactors MoreLikeThisQueryBuilder and Parser #13486

alexksikes · 2015-09-10T17:12:07Z

Relates to #10217

This PR is against the query-refactoring branch.

s1monw · 2015-09-11T15:47:18Z

@alexksikes can you rebase this to latest - there are quite some changes here that are now gone

Relates to elastic#10217 This PR is against the query-refactoring branch. Closes elastic#13486

alexksikes · 2015-09-14T15:37:20Z

@s1monw You can take a look now. Thank you.

colings86 · 2015-09-15T09:07:50Z

core/src/main/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilder.java

@@ -147,7 +204,8 @@ public Item doc(BytesReference doc) {
         * Sets to a given artificial document, that is a document that is not present in the index.
         */
        public Item doc(XContentBuilder doc) {
-            return this.doc(doc.bytes());
+            this.doc = doc != null ? doc.bytes() : null;


I don't think we should allow null here I think this should throw an exception if null is passed in

In fact, given that we require either doc or an index document id, this should really be a constructor and this setter should be removed. That way the user can only specify doc OR id and must specify one of them. We should add null checks in the constructors and throw IllegalArgumentException if nulls are passed in

Also, given that type and index are required if id is specified we should remove the setters for type, id and index and remove the zero-arg constructor for Item so the user is forced to correct initialise the object with either the index, type and id, or the index, type and doc

There is a constructor for doc. However, we do have this method for bwc, so maybe we should keep it?

Concerning null index, or type, this is so that these could be initialized by the search context, and we only have this context when the query is actually being created.

We don't need to keep the constructors/methods for bwc. For the Java API we are able to break backwards compatibility here and we should if it makes the use of the API less trappy (as it does here).

OK then if we are allowed to break bwc, then I much prefer to remove this method. That would simplify things from code and user perspective. A definite +1.

colings86 · 2015-09-15T09:26:09Z

@alexksikes I left some comments. I also think someone else will need to quickly look before its merged as I have no experience of the MLT query

alexksikes · 2015-09-15T09:47:56Z

@colings86 Thanks for the review. I'll address the comments on the next round of review.

cbuescher · 2015-09-15T12:28:38Z

core/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java

@@ -316,6 +316,18 @@ public void writeStringArrayNullable(@Nullable String[] array) throws IOExceptio
        }
    }

+    /**
+     * Writes a string array, for nullable string, writes it as 0 (empty string).
+     */


Comment seems wrong, as far as I can see the difference to writeStringArrayNullable is that it writes boolean instead of int in case of null argument.

why is this method needed if we have writeStringArrayNullable?

That one doesn't seem to differentiate between null and zero-length array, which we need if we e.g. test for equality of queries after serialization. At least that's what I think, @Ale might have had other reasons.

This exactly the reason. I first tried writeStringArrayNullable, but then you read back an empty array, not a null array.

I'm not a fan of having both of these methods. The difference between them is too subtle. I think we should instead replace writeStringArrayNullable with writeOptionalStringArray as the former is trappy and could easily lead to bugs. Serialisation code should always serialise and de-serialise to the same thing.

+1 should this change be part of this PR? Looks like this is not used anywhere else.

If it's not used anywhere else (or even if its only used in a couple of places) then I would be +1 to doing it in this PR

My bad this is actually used in a couple of places. I'll open an issue on this.

cbuescher · 2015-09-15T15:08:41Z

@alexksikes I did a round of reviews, given the amount of complexity in constructing the final MLT query I didn't check the internals of the helper methods moved over from the parsers, maybe those can be simplified later but as far as separating the toQuery/fromXContent I think this is good. Maybe someone else should have final look after you adressed the last comments.

alexksikes · 2015-09-15T16:15:56Z

@cbuescher @colings86 I addressed all the comments. Should I do the package change for the Item class as part of this PR too? Thanks again for the review.

s1monw · 2015-09-15T20:04:25Z

core/src/main/java/org/elasticsearch/index/VersionType.java

+
+    @Override
+    public VersionType readFrom(StreamInput in) throws IOException {
+        return VersionType.values()[in.readVInt()];


can we check the value we are reading here? an assertion would be ok

s1monw · 2015-09-15T20:04:39Z

core/src/main/java/org/elasticsearch/index/VersionType.java

+
+    @Override
+    public void writeTo(StreamOutput out) throws IOException {
+        out.writeVInt(this.ordinal());


this is unnecessary

s1monw · 2015-09-15T20:15:45Z

I left some minor comments LGTM otherwise

Relates to #10217 This PR is against the query-refactoring branch. Closes #13486

alexksikes added :Query Refactoring v5.0.0-alpha1 labels Sep 10, 2015

clintongormley mentioned this pull request Sep 11, 2015

Refactor parsing of queries/filters, aggs, suggester APIs #10217

Closed

s1monw self-assigned this Sep 11, 2015

Refactors MoreLikeThisQueryBuilder and Parser

d722c47

Relates to elastic#10217 This PR is against the query-refactoring branch. Closes elastic#13486

alexksikes force-pushed the feature/query-refactoring-more-like-this branch from 9c1a75c to d722c47 Compare September 12, 2015 16:22

alexksikes added 5 commits September 13, 2015 22:30

adds serialization and a couple of fixes

96f3b28

adds most builder tests

f060dbe

adds item validation

c00cac3

adds getters

3d817f4

adds serialization test for artificial doc and per field analyzer

9239c7c

alexksikes added the review label Sep 14, 2015

minor change Item#doc(XContentBuilder)

5d76514

colings86 reviewed Sep 15, 2015
View reviewed changes

cbuescher reviewed Sep 15, 2015
View reviewed changes

alexksikes added 2 commits September 15, 2015 18:10

addressed comments

c9ffe10

updated migration doc

bad2164

s1monw reviewed Sep 15, 2015
View reviewed changes

addressed comments

102c16b

alexksikes added a commit that referenced this pull request Sep 16, 2015

Refactors MoreLikeThisQueryBuilder and Parser

a13336d

Relates to #10217 This PR is against the query-refactoring branch. Closes #13486

alexksikes removed the review label Sep 16, 2015

alexksikes closed this Sep 16, 2015

alexksikes deleted the feature/query-refactoring-more-like-this branch September 16, 2015 00:20

javanna removed the v5.0.0-alpha1 label Sep 25, 2015

clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Query Refactoring labels Feb 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors MoreLikeThisQueryBuilder and Parser #13486

Refactors MoreLikeThisQueryBuilder and Parser #13486

alexksikes commented Sep 10, 2015

s1monw commented Sep 11, 2015

alexksikes commented Sep 14, 2015

colings86 Sep 15, 2015

colings86 Sep 15, 2015

colings86 Sep 15, 2015

alexksikes Sep 15, 2015

colings86 Sep 15, 2015

alexksikes Sep 15, 2015

colings86 commented Sep 15, 2015

alexksikes commented Sep 15, 2015

cbuescher Sep 15, 2015

colings86 Sep 15, 2015

cbuescher Sep 15, 2015

alexksikes Sep 15, 2015

colings86 Sep 15, 2015

alexksikes Sep 15, 2015

colings86 Sep 15, 2015

alexksikes Sep 15, 2015

cbuescher commented Sep 15, 2015

alexksikes commented Sep 15, 2015

s1monw Sep 15, 2015

s1monw Sep 15, 2015

s1monw commented Sep 15, 2015

Refactors MoreLikeThisQueryBuilder and Parser #13486

Refactors MoreLikeThisQueryBuilder and Parser #13486

Conversation

alexksikes commented Sep 10, 2015

s1monw commented Sep 11, 2015

alexksikes commented Sep 14, 2015

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

colings86 commented Sep 15, 2015

alexksikes commented Sep 15, 2015

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

cbuescher commented Sep 15, 2015

alexksikes commented Sep 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1monw commented Sep 15, 2015