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

Enable collapse on unsigned_long field #63495

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 @@ -291,6 +291,11 @@ protected BytesRef indexedValueForSearch(Object value) {
}
return getTextSearchInfo().getSearchAnalyzer().normalize(name(), value.toString());
}

@Override
public CollapseType collapseType() {
return CollapseType.KEYWORD;
}
}

private final boolean indexed;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,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. */
Expand Down Expand Up @@ -400,4 +409,10 @@ public Map<String, String> meta() {
public TextSearchInfo getTextSearchInfo() {
return textSearchInfo;
}

public enum CollapseType {
NONE, // this field is not collapsable
KEYWORD,
NUMERIC
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -203,12 +202,9 @@ 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 on this field type");
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep adding the field name to the error message to make it more actionable? Maybe we also have the actual field type available here to add to the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbuescher Thanks for the feedback, addressed in 621992c

}

if (fieldType.hasDocValues() == false) {
throw new IllegalArgumentException("cannot collapse on field `" + field + "` without `doc_values`");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 on this field type");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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 }