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.

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

[Remove] Type from TermsLookUp #2459

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
search:
rest_total_hits_as_int: true
index: "search_index"
body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "type" : "_doc", "id": "1", "path": "followers"} } } }
body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "id": "1", "path": "followers"} } } }
- do:
indices.create:
index: lookup_index
Expand All @@ -64,7 +64,7 @@
search:
rest_total_hits_as_int: true
index: "search_index"
body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "type" : "_doc", "id": "1", "path": "followers"} } } }
body: { "size" : 0, "query" : { "terms" : { "user" : { "index": "lookup_index", "id": "1", "path": "followers"} } } }

- match: { _shards.total: 5 }
- match: { _shards.successful: 5 }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1195,75 +1195,63 @@ public void testTermsLookupFilter() throws Exception {
);

SearchResponse searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "1", "terms")))
.setQuery(termsLookupQuery("term", new TermsLookup("lookup", "1", "terms")))
.get();
assertHitCount(searchResponse, 2L);
assertSearchHits(searchResponse, "1", "3");

// same as above, just on the _id...
searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("_id", new TermsLookup("lookup", "type", "1", "terms")))
.get();
searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("_id", new TermsLookup("lookup", "1", "terms"))).get();
assertHitCount(searchResponse, 2L);
assertSearchHits(searchResponse, "1", "3");

// another search with same parameters...
searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "1", "terms")))
.get();
searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup", "1", "terms"))).get();
assertHitCount(searchResponse, 2L);
assertSearchHits(searchResponse, "1", "3");

searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "2", "terms")))
.get();
searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup", "2", "terms"))).get();
assertHitCount(searchResponse, 1L);
assertFirstHit(searchResponse, hasId("2"));

searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "3", "terms")))
.get();
searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup", "3", "terms"))).get();
assertHitCount(searchResponse, 2L);
assertSearchHits(searchResponse, "2", "4");

searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "4", "terms")))
.get();
searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup", "4", "terms"))).get();
assertHitCount(searchResponse, 0L);

searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "type", "1", "arr.term")))
.setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "1", "arr.term")))
.get();
assertHitCount(searchResponse, 2L);
assertSearchHits(searchResponse, "1", "3");

searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "type", "2", "arr.term")))
.setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "2", "arr.term")))
.get();
assertHitCount(searchResponse, 1L);
assertFirstHit(searchResponse, hasId("2"));

searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "type", "3", "arr.term")))
.setQuery(termsLookupQuery("term", new TermsLookup("lookup2", "3", "arr.term")))
.get();
assertHitCount(searchResponse, 2L);
assertSearchHits(searchResponse, "2", "4");

searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("not_exists", new TermsLookup("lookup2", "type", "3", "arr.term")))
.setQuery(termsLookupQuery("not_exists", new TermsLookup("lookup2", "3", "arr.term")))
.get();
assertHitCount(searchResponse, 0L);

// index "lookup" type "type" id "missing" document does not exist: ignore the lookup terms
searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("term", new TermsLookup("lookup", "type", "missing", "terms")))
.setQuery(termsLookupQuery("term", new TermsLookup("lookup", "missing", "terms")))
.get();
assertHitCount(searchResponse, 0L);

// index "lookup3" type "type" has the source disabled: ignore the lookup terms
searchResponse = client().prepareSearch("test")
.setQuery(termsLookupQuery("term", new TermsLookup("lookup3", "type", "1", "terms")))
.get();
searchResponse = client().prepareSearch("test").setQuery(termsLookupQuery("term", new TermsLookup("lookup3", "1", "terms"))).get();
assertHitCount(searchResponse, 0L);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ public void testExplainTermsQueryWithLookup() throws Exception {
client().prepareIndex("twitter").setId("1").setSource("followers", new int[] { 1, 2, 3 }).get();
refresh();

TermsQueryBuilder termsLookupQuery = QueryBuilders.termsLookupQuery("user", new TermsLookup("twitter", "_doc", "1", "followers"));
TermsQueryBuilder termsLookupQuery = QueryBuilders.termsLookupQuery("user", new TermsLookup("twitter", "1", "followers"));
ValidateQueryResponse response = client().admin()
.indices()
.prepareValidateQuery("twitter")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,6 @@ public TermsLookup termsLookup() {
return this.termsLookup;
}

public boolean isTypeless() {
return termsLookup == null || termsLookup.type() == null;
}

private static final Set<Class<? extends Number>> INTEGER_TYPES = new HashSet<>(
Arrays.asList(Byte.class, Short.class, Integer.class, Long.class)
);
Expand Down
62 changes: 11 additions & 51 deletions server/src/main/java/org/opensearch/indices/TermsLookup.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@

package org.opensearch.indices;

import org.opensearch.LegacyESVersion;
import org.opensearch.common.Nullable;
import org.opensearch.Version;
import org.opensearch.common.ParseField;
import org.opensearch.common.io.stream.StreamInput;
import org.opensearch.common.io.stream.StreamOutput;
Expand All @@ -42,34 +41,25 @@
import org.opensearch.common.xcontent.ToXContentFragment;
import org.opensearch.common.xcontent.XContentBuilder;
import org.opensearch.common.xcontent.XContentParser;
import org.opensearch.index.mapper.MapperService;
import org.opensearch.index.query.TermsQueryBuilder;

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

import static org.opensearch.common.xcontent.ConstructingObjectParser.constructorArg;
import static org.opensearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;

/**
* Encapsulates the parameters needed to fetch terms.
*/
public class TermsLookup implements Writeable, ToXContentFragment {

private final String index;
private @Nullable String type;
private final String id;
private final String path;
private String routing;

public TermsLookup(String index, String id, String path) {
this(index, null, id, path);
}

/**
* @deprecated Types are in the process of being removed, use {@link TermsLookup(String, String, String)} instead.
*/
@Deprecated
public TermsLookup(String index, String type, String id, String path) {
if (id == null) {
throw new IllegalArgumentException("[" + TermsQueryBuilder.NAME + "] query lookup element requires specifying the id.");
}
Expand All @@ -80,7 +70,6 @@ public TermsLookup(String index, String type, String id, String path) {
throw new IllegalArgumentException("[" + TermsQueryBuilder.NAME + "] query lookup element requires specifying the index.");
}
this.index = index;
this.type = type;
this.id = id;
this.path = path;
}
Expand All @@ -89,11 +78,8 @@ public TermsLookup(String index, String type, String id, String path) {
* Read from a stream.
*/
public TermsLookup(StreamInput in) throws IOException {
if (in.getVersion().onOrAfter(LegacyESVersion.V_7_0_0)) {
type = in.readOptionalString();
} else {
// Before 7.0, the type parameter was always non-null and serialized as a (non-optional) string.
type = in.readString();
if (in.getVersion().before(Version.V_2_0_0)) {
in.readOptionalString();
}
id = in.readString();
path = in.readString();
Expand All @@ -103,16 +89,8 @@ public TermsLookup(StreamInput in) throws IOException {

@Override
public void writeTo(StreamOutput out) throws IOException {
if (out.getVersion().onOrAfter(LegacyESVersion.V_7_0_0)) {
out.writeOptionalString(type);
} else {
if (type == null) {
throw new IllegalArgumentException(
"Typeless [terms] lookup queries are not supported if any " + "node is running a version before 7.0."
);

}
out.writeString(type);
if (out.getVersion().before(Version.V_2_0_0)) {
out.writeOptionalString(MapperService.SINGLE_MAPPING_NAME);
}
out.writeString(id);
out.writeString(path);
Expand All @@ -124,14 +102,6 @@ public String index() {
return index;
}

/**
* @deprecated Types are in the process of being removed.
*/
@Deprecated
public String type() {
return type;
}

public String id() {
return id;
}
Expand All @@ -151,14 +121,12 @@ public TermsLookup routing(String routing) {

private static final ConstructingObjectParser<TermsLookup, Void> PARSER = new ConstructingObjectParser<>("terms_lookup", args -> {
String index = (String) args[0];
String type = (String) args[1];
String id = (String) args[2];
String path = (String) args[3];
return new TermsLookup(index, type, id, path);
String id = (String) args[1];
String path = (String) args[2];
return new TermsLookup(index, id, path);
});
static {
PARSER.declareString(constructorArg(), new ParseField("index"));
PARSER.declareString(optionalConstructorArg(), new ParseField("type").withAllDeprecated());
PARSER.declareString(constructorArg(), new ParseField("id"));
PARSER.declareString(constructorArg(), new ParseField("path"));
PARSER.declareString(TermsLookup::routing, new ParseField("routing"));
Expand All @@ -170,19 +138,12 @@ public static TermsLookup parseTermsLookup(XContentParser parser) throws IOExcep

@Override
public String toString() {
if (type == null) {
return index + "/" + id + "/" + path;
} else {
return index + "/" + type + "/" + id + "/" + path;
}
return index + "/" + id + "/" + path;
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.field("index", index);
if (type != null) {
builder.field("type", type);
}
builder.field("id", id);
builder.field("path", path);
if (routing != null) {
Expand All @@ -193,7 +154,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws

@Override
public int hashCode() {
return Objects.hash(index, type, id, path, routing);
return Objects.hash(index, id, path, routing);
}

@Override
Expand All @@ -206,7 +167,6 @@ public boolean equals(Object obj) {
}
TermsLookup other = (TermsLookup) obj;
return Objects.equals(index, other.index)
&& Objects.equals(type, other.type)
&& Objects.equals(id, other.id)
&& Objects.equals(path, other.path)
&& Objects.equals(routing, other.routing);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,7 @@ protected TermsQueryBuilder doCreateTestQueryBuilder() {

private TermsLookup randomTermsLookup() {
// Randomly choose between a typeless terms lookup and one with an explicit type to make sure we are
TermsLookup lookup = maybeIncludeType && randomBoolean()
? new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath)
: new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath);
TermsLookup lookup = new TermsLookup(randomAlphaOfLength(10), randomAlphaOfLength(10), termsPath);
// testing both cases.
lookup.routing(randomBoolean() ? randomAlphaOfLength(10) : null);
return lookup;
Expand Down Expand Up @@ -379,13 +377,6 @@ protected QueryBuilder parseQuery(XContentParser parser) throws IOException {
try {
QueryBuilder query = super.parseQuery(parser);
assertThat(query, CoreMatchers.instanceOf(TermsQueryBuilder.class));

TermsQueryBuilder termsQuery = (TermsQueryBuilder) query;
String deprecationWarning = "Deprecated field [type] used, this field is unused and will be removed entirely";
if (termsQuery.isTypeless() == false && !assertedWarnings.contains(deprecationWarning)) {
assertWarnings(deprecationWarning);
assertedWarnings.add(deprecationWarning);
}
return query;
} finally {

Expand Down
Loading