[Remove] Type from TermsLookUp (#2459)

* [Remove] Type from TermsLookUp Signed-off-by: Suraj Singh <[email protected]> * Fix unit test failure Signed-off-by: Suraj Singh <[email protected]>
opensearch-project · Mar 15, 2022 · 2b68b14 · 2b68b14
1 parent 5c0f9bc
commit 2b68b14
Show file tree

Hide file tree

Showing 7 changed files with 32 additions and 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
@@ -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
@@ -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);
     }
 

diff --git 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 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")

diff --git 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 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)
     );

diff --git 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,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.");
         }
@@ -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;
     }
@@ -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();
@@ -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);
@@ -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;
     }
@@ -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"));
@@ -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) {
@@ -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
@@ -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);

diff --git 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 @@ 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;
@@ -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 {