From 2b68b14629ba8bb5ccb1db3ed3806f837b56df15 Mon Sep 17 00:00:00 2001 From: Suraj Singh <79435743+dreamer-89@users.noreply.github.com> Date: Mon, 14 Mar 2022 19:09:04 -0700 Subject: [PATCH] [Remove] Type from TermsLookUp (#2459) * [Remove] Type from TermsLookUp Signed-off-by: Suraj Singh * Fix unit test failure Signed-off-by: Suraj Singh --- .../search/150_rewrite_on_coordinator.yml | 4 +- .../search/query/SearchQueryIT.java | 36 ++++------- .../validate/SimpleValidateQueryIT.java | 2 +- .../index/query/TermsQueryBuilder.java | 4 -- .../org/opensearch/indices/TermsLookup.java | 62 ++++--------------- .../index/query/TermsQueryBuilderTests.java | 11 +--- .../opensearch/indices/TermsLookupTests.java | 55 ++-------------- 7 files changed, 32 insertions(+), 142 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/150_rewrite_on_coordinator.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/150_rewrite_on_coordinator.yml index be34e10ddcd..77298cb4f61 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search/150_rewrite_on_coordinator.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search/150_rewrite_on_coordinator.yml @@ -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 @@ -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 } diff --git a/server/src/internalClusterTest/java/org/opensearch/search/query/SearchQueryIT.java b/server/src/internalClusterTest/java/org/opensearch/search/query/SearchQueryIT.java index db87269c8ce..c9bb7469732 100644 --- a/server/src/internalClusterTest/java/org/opensearch/search/query/SearchQueryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/search/query/SearchQueryIT.java @@ -1195,75 +1195,63 @@ public class SearchQueryIT extends OpenSearchIntegTestCase { ); 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); } diff --git a/server/src/internalClusterTest/java/org/opensearch/validate/SimpleValidateQueryIT.java b/server/src/internalClusterTest/java/org/opensearch/validate/SimpleValidateQueryIT.java index 29845b39bec..30ab282bf3d 100644 --- a/server/src/internalClusterTest/java/org/opensearch/validate/SimpleValidateQueryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/validate/SimpleValidateQueryIT.java @@ -491,7 +491,7 @@ public class SimpleValidateQueryIT extends OpenSearchIntegTestCase { 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") diff --git a/server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java b/server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java index e797730ac0d..ac29cb2cf52 100644 --- a/server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java +++ b/server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java @@ -225,10 +225,6 @@ public class TermsQueryBuilder extends AbstractQueryBuilder { return this.termsLookup; } - public boolean isTypeless() { - return termsLookup == null || termsLookup.type() == null; - } - private static final Set> INTEGER_TYPES = new HashSet<>( Arrays.asList(Byte.class, Short.class, Integer.class, Long.class) ); diff --git a/server/src/main/java/org/opensearch/indices/TermsLookup.java b/server/src/main/java/org/opensearch/indices/TermsLookup.java index 1aa16ad5cd7..bf6c024fa13 100644 --- a/server/src/main/java/org/opensearch/indices/TermsLookup.java +++ b/server/src/main/java/org/opensearch/indices/TermsLookup.java @@ -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; @@ -42,13 +41,13 @@ import org.opensearch.common.xcontent.ConstructingObjectParser; 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. @@ -56,20 +55,11 @@ import static org.opensearch.common.xcontent.ConstructingObjectParser.optionalCo 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."); } @@ -80,7 +70,6 @@ public class TermsLookup implements Writeable, ToXContentFragment { throw new IllegalArgumentException("[" + TermsQueryBuilder.NAME + "] query lookup element requires specifying the index."); } this.index = index; - this.type = type; this.id = id; this.path = path; } @@ -89,11 +78,8 @@ public class TermsLookup implements Writeable, ToXContentFragment { * 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(); @@ -103,16 +89,8 @@ public class TermsLookup implements Writeable, ToXContentFragment { @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); @@ -124,14 +102,6 @@ public class TermsLookup implements Writeable, ToXContentFragment { return index; } - /** - * @deprecated Types are in the process of being removed. - */ - @Deprecated - public String type() { - return type; - } - public String id() { return id; } @@ -151,14 +121,12 @@ public class TermsLookup implements Writeable, ToXContentFragment { private static final ConstructingObjectParser 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")); @@ -170,19 +138,12 @@ public class TermsLookup implements Writeable, ToXContentFragment { @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) { @@ -193,7 +154,7 @@ public class TermsLookup implements Writeable, ToXContentFragment { @Override public int hashCode() { - return Objects.hash(index, type, id, path, routing); + return Objects.hash(index, id, path, routing); } @Override @@ -206,7 +167,6 @@ public class TermsLookup implements Writeable, ToXContentFragment { } 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); diff --git a/server/src/test/java/org/opensearch/index/query/TermsQueryBuilderTests.java b/server/src/test/java/org/opensearch/index/query/TermsQueryBuilderTests.java index e37b4f1a1c3..ea93d7a65b9 100644 --- a/server/src/test/java/org/opensearch/index/query/TermsQueryBuilderTests.java +++ b/server/src/test/java/org/opensearch/index/query/TermsQueryBuilderTests.java @@ -119,9 +119,7 @@ public class TermsQueryBuilderTests extends AbstractQueryTestCase