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

Refactors MoreLikeThisQueryBuilder and Parser #13486

Closed
wants to merge 10 commits into from
+950 −485
Closed

Refactors MoreLikeThisQueryBuilder and Parser #13486

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d722c47
Refactors MoreLikeThisQueryBuilder and Parser
alexksikes Sep 10, 2015
96f3b28
adds serialization and a couple of fixes
alexksikes Sep 13, 2015
f060dbe
adds most builder tests
alexksikes Sep 14, 2015
c00cac3
adds item validation
alexksikes Sep 14, 2015
3d817f4
adds getters
alexksikes Sep 14, 2015
9239c7c
adds serialization test for artificial doc and per field analyzer
alexksikes Sep 14, 2015
5d76514
minor change Item#doc(XContentBuilder)
alexksikes Sep 14, 2015
c9ffe10
addressed comments
alexksikes Sep 15, 2015
bad2164
updated migration doc
alexksikes Sep 15, 2015
102c16b
addressed comments
alexksikes Sep 16, 2015
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
7 changes: 7 additions & 0 deletions core/src/main/java/org/elasticsearch/common/io/stream/StreamInput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,13 @@ public String[] readStringArray() throws IOException {
return ret;
}

public String[] readOptionalStringArray() throws IOException {
if (readBoolean()) {
return readStringArray();
}
return null;
}

@Nullable
@SuppressWarnings("unchecked")
public Map<String, Object> readMap() throws IOException {
Expand Down
12 changes: 12 additions & 0 deletions core/src/main/java/org/elasticsearch/common/io/stream/StreamOutput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
*/
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

why is this method needed if we have writeStringArrayNullable?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

public void writeOptionalStringArray(@Nullable String[] array) throws IOException {
if (array == null) {
writeBoolean(false);
} else {
writeBoolean(true);
writeStringArray(array);
}
}

public void writeMap(@Nullable Map<String, Object> map) throws IOException {
writeGenericValue(map);
}
Expand Down
23 changes: 22 additions & 1 deletion core/src/main/java/org/elasticsearch/index/VersionType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,17 @@
*/
package org.elasticsearch.index;

import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.lucene.uid.Versions;

import java.io.IOException;

/**
*
*/
public enum VersionType {
public enum VersionType implements Writeable<VersionType> {
INTERNAL((byte) 0) {
@Override
public boolean isVersionConflictForWrites(long currentVersion, long expectedVersion) {
Expand Down Expand Up @@ -219,6 +224,8 @@ public boolean validateVersionForReads(long version) {

private final byte value;

private static final VersionType PROTOTYPE = INTERNAL;

VersionType(byte value) {
this.value = value;
}
Expand Down Expand Up @@ -304,4 +311,18 @@ public static VersionType fromValue(byte value) {
}
throw new IllegalArgumentException("No version type match [" + value + "]");
}

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

Choose a reason for hiding this comment

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

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

}

public static VersionType readVersionTypeFrom(StreamInput in) throws IOException {
return PROTOTYPE.readFrom(in);
}

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

Choose a reason for hiding this comment

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

this is unnecessary

}
}
Loading