Enable collapse on unsigned_long field (#63495) (#63810)

Collapse was not working on unsigned_long field, as collapsing was enabled only on KeywordFieldType and NumberFieldType. This introduces a new method `collapseType` to MappedFieldType, that is checked to decide if collapsing should be enabled. Relates to #60050
elastic · Oct 16, 2020 · 7e13c56 · 7e13c56
1 parent 356109f
commit 7e13c56
Show file tree

Hide file tree

Showing 8 changed files with 120 additions and 15 deletions.
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java
@@ -294,6 +294,11 @@ protected BytesRef indexedValueForSearch(Object value) {
             }
             return getTextSearchInfo().getSearchAnalyzer().normalize(name(), value.toString());
         }
+
+        @Override
+        public CollapseType collapseType() {
+            return CollapseType.KEYWORD;
+        }
     }
 
     private final boolean indexed;

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java
@@ -141,6 +141,15 @@ public void setIndexAnalyzer(NamedAnalyzer analyzer) {
         this.indexAnalyzer = analyzer;
     }
 
+    /**
+     * Returns the collapse type of the field
+     * CollapseType.NONE means the field can'be used for collapsing.
+     * @return collapse type of the field
+     */
+    public CollapseType collapseType() {
+        return CollapseType.NONE;
+    }
+
     /** Given a value that comes from the stored fields API, convert it to the
      *  expected type. For instance a date field would store dates as longs and
      *  format it back to a string in this method. */
@@ -415,4 +424,10 @@ public Map<String, String> meta() {
     public TextSearchInfo getTextSearchInfo() {
         return textSearchInfo;
     }
+
+    public enum CollapseType {
+        NONE, // this field is not collapsable
+        KEYWORD,
+        NUMERIC
+    }
 }
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
@@ -1007,6 +1007,11 @@ public DocValueFormat docValueFormat(String format, ZoneId timeZone) {
         public Number parsePoint(byte[] value) {
             return type.parsePoint(value);
         }
+
+        @Override
+        public CollapseType collapseType() {
+            return CollapseType.NUMERIC;
+        }
     }
 
     private final NumberType type;

diff --git a/server/src/main/java/org/elasticsearch/search/collapse/CollapseBuilder.java b/server/src/main/java/org/elasticsearch/search/collapse/CollapseBuilder.java
@@ -29,11 +29,10 @@
 import org.elasticsearch.common.xcontent.ToXContentObject;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
-import org.elasticsearch.index.mapper.KeywordFieldMapper;
 import org.elasticsearch.index.mapper.MappedFieldType;
-import org.elasticsearch.index.mapper.NumberFieldMapper;
 import org.elasticsearch.index.query.InnerHitBuilder;
 import org.elasticsearch.index.query.QueryShardContext;
+import org.elasticsearch.index.mapper.MappedFieldType.CollapseType;
 
 import java.io.IOException;
 import java.util.ArrayList;
@@ -203,12 +202,10 @@ public CollapseContext build(QueryShardContext queryShardContext) {
         if (fieldType == null) {
             throw new IllegalArgumentException("no mapping found for `" + field + "` in order to collapse on");
         }
-        if (fieldType instanceof KeywordFieldMapper.KeywordFieldType == false &&
-            fieldType instanceof NumberFieldMapper.NumberFieldType == false) {
-            throw new IllegalArgumentException("unknown type for collapse field `" + field +
-                "`, only keywords and numbers are accepted");
+        if (fieldType.collapseType() == CollapseType.NONE) {
+            throw new IllegalArgumentException("collapse is not supported for the field [" + fieldType.name() +
+                "] of the type [" + fieldType.typeName() + "]");
         }
-
         if (fieldType.hasDocValues() == false) {
             throw new IllegalArgumentException("cannot collapse on field `" + field + "` without `doc_values`");
         }

diff --git a/server/src/main/java/org/elasticsearch/search/collapse/CollapseContext.java b/server/src/main/java/org/elasticsearch/search/collapse/CollapseContext.java
@@ -20,10 +20,9 @@
 
 import org.apache.lucene.search.Sort;
 import org.apache.lucene.search.grouping.CollapsingTopDocsCollector;
-import org.elasticsearch.index.mapper.KeywordFieldMapper;
 import org.elasticsearch.index.mapper.MappedFieldType;
-import org.elasticsearch.index.mapper.NumberFieldMapper;
 import org.elasticsearch.index.query.InnerHitBuilder;
+import org.elasticsearch.index.mapper.MappedFieldType.CollapseType;
 
 import java.util.List;
 
@@ -61,13 +60,12 @@ public List<InnerHitBuilder> getInnerHit() {
     }
 
     public CollapsingTopDocsCollector<?> createTopDocs(Sort sort, int topN) {
-        if (fieldType instanceof KeywordFieldMapper.KeywordFieldType) {
+        if (fieldType.collapseType() == CollapseType.KEYWORD) {
             return CollapsingTopDocsCollector.createKeyword(fieldName, fieldType, sort, topN);
-        } else if (fieldType instanceof NumberFieldMapper.NumberFieldType) {
+        } else if (fieldType.collapseType() == CollapseType.NUMERIC) {
             return CollapsingTopDocsCollector.createNumeric(fieldName, fieldType, sort, topN);
         } else {
-            throw new IllegalStateException("unknown type for collapse field " + fieldName +
-                ", only keywords and numbers are accepted");
+            throw new IllegalStateException("collapse is not supported on this field type");
         }
     }
 }
diff --git a/server/src/test/java/org/elasticsearch/search/collapse/CollapseBuilderTests.java b/server/src/test/java/org/elasticsearch/search/collapse/CollapseBuilderTests.java
@@ -205,7 +205,7 @@ public void testBuildWithExceptions() {
             MappedFieldType fieldType = new MappedFieldType("field", true, false, true, TextSearchInfo.NONE, Collections.emptyMap()) {
                 @Override
                 public String typeName() {
-                    return null;
+                    return "some_type";
                 }
 
                 @Override
@@ -225,7 +225,7 @@ public Query existsQuery(QueryShardContext context) {
             when(shardContext.getFieldType("field")).thenReturn(fieldType);
             CollapseBuilder builder = new CollapseBuilder("field");
             IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> builder.build(shardContext));
-            assertEquals(exc.getMessage(), "unknown type for collapse field `field`, only keywords and numbers are accepted");
+            assertEquals(exc.getMessage(), "collapse is not supported for the field [field] of the type [some_type]");
         }
     }
 

diff --git a/...gned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongFieldMapper.java b/...gned-long/src/main/java/org/elasticsearch/xpack/unsignedlong/UnsignedLongFieldMapper.java
@@ -277,6 +277,11 @@ public Function<byte[], Number> pointReaderIfPossible() {
             return null;
         }
 
+        @Override
+        public CollapseType collapseType() {
+            return CollapseType.NUMERIC;
+        }
+
         /**
          * Parses value to unsigned long for Term Query
          * @param value to to parse

diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/unsigned_long/60_collapse.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/unsigned_long/60_collapse.yml
@@ -0,0 +1,80 @@
+setup:
+
+  - skip:
+      version: " - 7.99.99"
+      reason: "collapse on unsigned_long was added in 8.0"
+
+  - do:
+      indices.create:
+        index:  test1
+        body:
+          settings:
+            index.number_of_shards: 3
+          mappings:
+            properties:
+              ul:
+                type: unsigned_long
+              k:
+                type: keyword
+
+  - do:
+      bulk:
+        index: test1
+        refresh: true
+        body: |
+          { "index": {"_id" : "1"} }
+          { "ul": 0, "k": "01" }
+          { "index": {"_id" : "2"} }
+          { "ul": 0, "k": "02" }
+          { "index": {"_id" : "3"} }
+          { "ul": 9223372036854775807, "k": "03" }
+          { "index": {"_id" : "4"} }
+          { "ul": 9223372036854775807, "k": "04" }
+          { "index": {"_id" : "5"} }
+          { "ul": 9223372036854775808, "k": "05" }
+          { "index": {"_id" : "6"} }
+          { "ul": 9223372036854775808, "k": "06" }
+          { "index": {"_id" : "7"} }
+          { "ul": 18446744073709551614, "k": "07" }
+          { "index": {"_id" : "8"} }
+          { "ul": 18446744073709551615, "k": "08" }
+          { "index": {"_id" : "9"} }
+          { "ul": 18446744073709551615, "k": "09" }
+          { "index": {"_id" : "10"} }
+          { "ul": 18446744073709551615, "k": "10" }
+
+---
+"Collapse":
+  - do:
+      search:
+        index: test1
+        body:
+          collapse:
+            field: ul
+            inner_hits:
+              name: my_inner_hits
+              _source : false
+              size: 3
+          sort: [ "k" ]
+
+  - match: { hits.total.value: 10 }
+
+  - match: { hits.hits.0._id: "1" }
+  - match: { hits.hits.0.fields.ul: [0] }
+  - match: { hits.hits.0.inner_hits.my_inner_hits.hits.total.value: 2 }
+
+  - match: { hits.hits.1._id: "3" }
+  - match: { hits.hits.1.fields.ul: [9223372036854775807] }
+  - match: { hits.hits.1.inner_hits.my_inner_hits.hits.total.value: 2 }
+
+  - match: { hits.hits.2._id: "5" }
+  - match: { hits.hits.2.fields.ul: [9223372036854775808] }
+  - match: { hits.hits.2.inner_hits.my_inner_hits.hits.total.value: 2 }
+
+  - match: { hits.hits.3._id: "7" }
+  - match: { hits.hits.3.fields.ul: [18446744073709551614] }
+  - match: { hits.hits.3.inner_hits.my_inner_hits.hits.total.value: 1 }
+
+  - match: { hits.hits.4._id: "8" }
+  - match: { hits.hits.4.fields.ul: [18446744073709551615] }
+  - match: { hits.hits.4.inner_hits.my_inner_hits.hits.total.value: 3 }