Skip to content

Refactor MatchAllQueryBuilder, TermQueryBuilder, IdsQueryBuilder #10454

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

Closed
wants to merge 4 commits into from
Closed

Refactor MatchAllQueryBuilder, TermQueryBuilder, IdsQueryBuilder #10454

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b031003
Split parse method for MatchAllQuery and TermQuery, add tests
Apr 1, 2015
cff9d73
Added equals, hashCode to MatchAllQuery, TermQuery. Added IdsQueryBui…
Apr 2, 2015
77cf6fe
Add tests to TermQueryBuilder
Apr 9, 2015
946fa37
Addressed comments, changed getter names, simplyfied test code
Apr 10, 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
116 changes: 98 additions & 18 deletions src/main/java/org/elasticsearch/index/query/IdsQueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
import org.apache.lucene.search.Query;
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.io.stream.Streamable;
import org.elasticsearch.common.lucene.search.Queries;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
Expand All @@ -37,30 +40,29 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.Objects;

/**
* A query that will return only documents matching specific ids (and a type).
*/
public class IdsQueryBuilder extends BaseQueryBuilder implements QueryParser, BoostableQueryBuilder<IdsQueryBuilder> {
public class IdsQueryBuilder extends BaseQueryBuilder implements QueryParser, BoostableQueryBuilder<IdsQueryBuilder>, Streamable {

private final List<String> types;
private Collection<String> types = new ArrayList<String>();
Copy link
Member

Choose a reason for hiding this comment

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

new ArrayList() => new ArrayList<>()


private List<String> values = new ArrayList<>();
private Collection<String> values = new ArrayList<String>();
Copy link
Member

Choose a reason for hiding this comment

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

same as above


private float boost = -1;
private float boost = 1.0f;

private String queryName;

public static final String NAME = "ids";

@Inject
public IdsQueryBuilder() {
this.types = null;
}

public IdsQueryBuilder(String... types) {
this.types = types == null ? null : Arrays.asList(types);
this.types = types == null ? new ArrayList<String>() : Arrays.asList(types);
}

/**
Expand All @@ -78,6 +80,10 @@ public IdsQueryBuilder ids(String... ids) {
return addIds(ids);
}

Collection<String> getIds() {
return this.values;
}

/**
* Sets the boost for this query. Documents matching this query will (in addition to the normal
* weightings) have their score multiplied by the boost provided.
Expand All @@ -88,6 +94,10 @@ public IdsQueryBuilder boost(float boost) {
return this;
}

public float getBoost() {
Copy link
Member

Choose a reason for hiding this comment

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

one more thing, these are going to be user facing public methods, we should add javadocs for each of them

return this.boost;
}

/**
* Sets the query name for the filter that can be used when searching for matched_filters per hit.
*/
Expand All @@ -101,7 +111,7 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep
builder.startObject(IdsQueryBuilder.NAME);
if (types != null) {
if (types.size() == 1) {
builder.field("type", types.get(0));
builder.field("type", types.iterator().next());
} else {
builder.startArray("types");
for (Object type : types) {
Expand All @@ -115,7 +125,7 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep
builder.value(value);
}
builder.endArray();
if (boost != -1) {
if (boost != 1.0f) {
builder.field("boost", boost);
}
if (queryName != null) {
Expand All @@ -128,16 +138,18 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep
public String[] names() {
return new String[]{NAME};
}

@Override
public Query parse(QueryParseContext parseContext) throws IOException, QueryParsingException {
IdsQueryBuilder query = new IdsQueryBuilder();
query.fromXContent(parseContext);
return query.toQuery(parseContext);
}

public void fromXContent(QueryParseContext parseContext) throws IOException {
XContentParser parser = parseContext.parser();

List<BytesRef> ids = new ArrayList<>();
Collection<String> types = null;
String currentFieldName = null;
float boost = 1.0f;
String queryName = null;
XContentParser.Token token;
boolean idsProvided = false;
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
Expand All @@ -149,18 +161,17 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
if ((token == XContentParser.Token.VALUE_STRING) ||
(token == XContentParser.Token.VALUE_NUMBER)) {
BytesRef value = parser.utf8BytesOrNull();
if (value == null) {
String id = parser.textOrNull();
if (id == null) {
throw new QueryParsingException(parseContext.index(), "No value specified for term filter");
}
ids.add(value);
values.add(id);
} else {
throw new QueryParsingException(parseContext.index(),
"Illegal value for id, expecting a string or number, got: " + token);
}
}
} else if ("types".equals(currentFieldName) || "type".equals(currentFieldName)) {
types = new ArrayList<>();
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
String value = parser.textOrNull();
if (value == null) {
Expand All @@ -187,7 +198,16 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
if (!idsProvided) {
throw new QueryParsingException(parseContext.index(), "[ids] query, no ids values provided");
}
}

public Query toQuery(QueryParseContext parseContext) throws IOException, QueryParsingException {
ArrayList<BytesRef> ids = new ArrayList<BytesRef>();

for (String value : this.values) {
BytesRef ref = new BytesRef(value);
ids.add(ref);
}

if (ids.isEmpty()) {
return Queries.newMatchNoDocsQuery();
}
Expand All @@ -207,4 +227,64 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
}
return query;
}

@Override
public void readFrom(StreamInput in) throws IOException {
int typeSize = in.readInt();
for (int i = 0; i < typeSize; i++) {
types.add(in.readString());
}
int valueSize = in.readInt();
for (int i = 0; i < valueSize; i++) {
values.add(in.readString());
}
queryName = in.readOptionalString();
boost = in.readFloat();
}

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeInt(types.size());
for (String type : types) {
out.writeString(type);
}
out.writeInt(values.size());
for (String value : values) {
out.writeString(value);
}
out.writeOptionalString(queryName);
out.writeFloat(boost);
}

@Override
public int hashCode() {
int hash = super.hashCode();
hash = maybeHashcode(hash, values);
hash = maybeHashcode(hash, types);
hash = maybeHashcode(hash, queryName);
hash = maybeHashcode(hash, boost);
return hash;
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
return true;
}
if (getClass() != obj.getClass()) {
return false;
}
IdsQueryBuilder other = (IdsQueryBuilder) obj;
return Objects.equals(values, other.values) &&
Objects.equals(types, other.types) &&
Objects.equals(boost, other.boost)&&
Objects.equals(queryName, other.queryName);
}

/**
* Return a prime (31) times the staring hash and object's hash, if non-null
*/
private int maybeHashcode(int startingHash, Object obj) {
return 31 * startingHash + ((obj == null) ? 0 : obj.hashCode());
}
}
93 changes: 67 additions & 26 deletions src/main/java/org/elasticsearch/index/query/MatchAllQueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,37 +23,29 @@
import org.apache.lucene.search.Query;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.io.stream.Streamable;
import org.elasticsearch.common.lucene.search.Queries;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;

import java.io.IOException;
import java.util.Objects;

/**
* A query that matches on all documents.
*
*
*/
public class MatchAllQueryBuilder extends BaseQueryBuilder implements QueryParser, BoostableQueryBuilder<MatchAllQueryBuilder> {

private String normsField;
public class MatchAllQueryBuilder extends BaseQueryBuilder implements QueryParser, BoostableQueryBuilder<MatchAllQueryBuilder>, Streamable {

private float boost = -1;
private float boost = 1.0f;

public static final String NAME = "match_all";

@Inject
public MatchAllQueryBuilder() {
}

/**
* Field used for normalization factor (document boost). Defaults to no field.
*/
public MatchAllQueryBuilder normsField(String normsField) {
this.normsField = normsField;
return this;
}

/**
* Sets the boost for this query. Documents matching this query will (in addition to the normal
* weightings) have their score multiplied by the boost provided.
Expand All @@ -64,15 +56,19 @@ public MatchAllQueryBuilder boost(float boost) {
return this;
}

/**
* @return the boost for this query.
*/
public float getBoost() {
Copy link
Member

Choose a reason for hiding this comment

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

If we want to go with pure getter, which is fine with me, we should probably change our setters too then for consistency? I think we have public MatchAllQueryBuilder boost(float boost) here. Not talking about the fluent thing, just the name of the method, either get and set or no get nor set prefix .

Copy link
Member Author

Choose a reason for hiding this comment

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

I also thought about making this package private, only used in test so far. Changing setter is also fine with me but will break client code somewhere I guess?

Copy link
Member

Choose a reason for hiding this comment

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

I think getters don't hurt and should have already been there. At the moment stuff can't be set but not retrieved, which seems silly. If we want to keep bw comp then let's leave the setter alone and follow the same convention with getters too

return this.boost;
}

@Override
public void doXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(NAME);
if (boost != -1) {
if (boost != 1.0f) {
builder.field("boost", boost);
}
if (normsField != null) {
builder.field("norms_field", normsField);
}
builder.endObject();
}

Expand All @@ -83,9 +79,27 @@ public String[] names() {

@Override
public Query parse(QueryParseContext parseContext) throws IOException, QueryParsingException {
MatchAllQueryBuilder query = new MatchAllQueryBuilder();
query.fromXContent(parseContext);
return query.toQuery(parseContext);
Copy link
Member

Choose a reason for hiding this comment

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

I guess we could generify this on the base class since the last two lines are going to be the same everywhere? and have a new abstract method newQuery() that creates a new instance of the query(Builder) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I thought so.
In the alternative I posted today I only have

@Override
        public Query parse(QueryParseContext parseContext) throws IOException, QueryParsingException {
            return fromXContent(parseContext).toQuery();
       }

master...cbuescher:proto/queryrefactoring-matchall-v2#diff-b5958c2f636285e4231fcbb0ef38ebf5R167

Copy link
Member

Choose a reason for hiding this comment

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

I have yet to get to that alternative, I am lagging behind :)

}

public Query toQuery(QueryParseContext parseContext) {
Copy link
Member

Choose a reason for hiding this comment

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

do we need the QueryParseContext argument here? we may need it in some other query?

if (boost == 1.0f) {
return Queries.newMatchAllQuery();
}

//LUCENE 4 UPGRADE norms field is not supported anymore need to find another way or drop the functionality
//MatchAllDocsQuery query = new MatchAllDocsQuery(normsField);
MatchAllDocsQuery query = new MatchAllDocsQuery();
Copy link
Member

Choose a reason for hiding this comment

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

this comment is there from ages. Given that normsField doesn't get parsed for ages either, we should just drop any reference of it I guess. Wondering if we should do that upstream though or here. Just scared that we make too many changes at the same time and we don't track them. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Going to drop this since it's not supported in lucene and there seems to be nothing on this option in the docs, nor is that field parsed anywhere.

Copy link
Member

Choose a reason for hiding this comment

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

agreed on dropping, which we kinda did already :) how do proceed though? upstream? in our branch? how we do we track of these unrelated changes (I fear there's going to be many given that the deeper you dig the more you find...)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can open issue for this upstream, discuss and fix it there, then revert this change here. Will be merged then later.

query.setBoost(boost);
return query;
}

public void fromXContent(QueryParseContext parseContext) throws IOException, QueryParsingException {
XContentParser parser = parseContext.parser();

float boost = 1.0f;
this.boost = 1.0f;
Copy link
Member

Choose a reason for hiding this comment

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

nitpick, you might want to add this. to the below reference of boost during parsing (not shown here unfortunately)?

String currentFieldName = null;

XContentParser.Token token;
Expand All @@ -100,15 +114,42 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
}
}
}
}

if (boost == 1.0f) {
return Queries.newMatchAllQuery();
@Override
public void readFrom(StreamInput in) throws IOException {
this.boost = in.readFloat();
}

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeFloat(this.boost);
}

@Override
public int hashCode() {
int hash = super.hashCode();
hash = maybeHashcode(hash, boost);
Copy link
Member

Choose a reason for hiding this comment

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

seems odd that we have to call a method that accepts Object passing in a float as argument. Can we simplify?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is copy from Lees PR, happy to simplify this in any way. Isabel mentioned utility functions in some google lib we are already using for this, will ask her about it.

return hash;
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
return true;
}
if (getClass() != obj.getClass()) {
return false;
}

//LUCENE 4 UPGRADE norms field is not supported anymore need to find another way or drop the functionality
//MatchAllDocsQuery query = new MatchAllDocsQuery(normsField);
MatchAllDocsQuery query = new MatchAllDocsQuery();
query.setBoost(boost);
return query;
MatchAllQueryBuilder other = (MatchAllQueryBuilder) obj;
return Objects.equals(boost, other.boost);
Copy link
Member

Choose a reason for hiding this comment

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

return boost == other.boost ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Did this to be consistent with Lees PR here https://github.com/elastic/elasticsearch/pull/10405/files#diff-08a3154601eec8465b50ac68b45c4c48R523.
Im happy to do this either way.

Copy link
Member

Choose a reason for hiding this comment

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

I see now... yea it's odd here cause this query has a single float field, looks better on more complex queries (especially cause you don't really see that among many fields)...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave this in for now, since it also takes care of null checks (although that shouldn't be possible for float boost here)

}

/**
* Return a prime (31) times the staring hash and object's hash, if non-null
*/
private int maybeHashcode(int startingHash, Object obj) {
return 31 * startingHash + ((obj == null) ? 0 : obj.hashCode());
}
}
Loading