-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Distinguish between simple matches with and without the terms index #63945
Changes from 4 commits
5e0c1dd
172c803
f2037bf
807bae1
1f81fbe
8714356
6adf89e
11fc9d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,7 +42,7 @@ static final class VersionFieldType extends MappedFieldType { | |
public static final VersionFieldType INSTANCE = new VersionFieldType(); | ||
|
||
private VersionFieldType() { | ||
super(NAME, false, false, true, TextSearchInfo.SIMPLE_MATCH_ONLY, Collections.emptyMap()); | ||
super(NAME, false, false, true, TextSearchInfo.NONE, Collections.emptyMap()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to have slipped when I double checked all the mappers, maybe it does not have a test so we don't enforce its consistency. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, there's no test for VersionFieldMapper - I'll add one as a follow up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thank you |
||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,7 @@ | |
import org.elasticsearch.common.util.BytesRefHash; | ||
import org.elasticsearch.common.util.ObjectArray; | ||
import org.elasticsearch.index.mapper.MappedFieldType; | ||
import org.elasticsearch.index.mapper.TextSearchInfo; | ||
import org.elasticsearch.index.query.QueryBuilder; | ||
import org.elasticsearch.search.DocValueFormat; | ||
import org.elasticsearch.search.aggregations.Aggregator; | ||
|
@@ -84,9 +85,9 @@ public SignificantTextAggregatorFactory(String name, | |
// Note that if the field is unmapped (its field type is null), we don't fail, | ||
// and just use the given field name as a placeholder. | ||
this.fieldType = context.getFieldType(fieldName); | ||
if (fieldType != null && fieldType.indexAnalyzer() == null) { | ||
throw new IllegalArgumentException("Field [" + fieldType.name() + "] has no analyzer, but SignificantText " + | ||
"requires an analyzed field"); | ||
if (fieldType != null && supportsAgg(fieldType) == false) { | ||
throw new IllegalArgumentException("Field [" + fieldType.name() + "] of type [" | ||
+ fieldType.typeName() + "] does not support significant text aggregations"); | ||
} | ||
this.indexedFieldName = fieldType != null ? fieldType.name() : fieldName; | ||
this.sourceFieldNames = sourceFieldNames == null ? new String[] { indexedFieldName } : sourceFieldNames; | ||
|
@@ -98,6 +99,13 @@ public SignificantTextAggregatorFactory(String name, | |
this.significanceHeuristic = significanceHeuristic; | ||
} | ||
|
||
private static boolean supportsAgg(MappedFieldType ft) { | ||
if (ft.getTextSearchInfo() == TextSearchInfo.NONE) { | ||
return false; | ||
} | ||
return ft.getTextSearchInfo() != TextSearchInfo.SIMPLE_MATCH_WITHOUT_TERMS; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should it be return ft.getTextSearchInfo != NONE && gt.getTextSearchIngo != SIMPLE_MATCH_WITHOUT_TERMS ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ++ |
||
} | ||
|
||
@Override | ||
protected Aggregator createInternal(SearchContext searchContext, Aggregator parent, CardinalityUpperBound cardinality, | ||
Map<String, Object> metadata) throws IOException { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we lose coverage with these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, you're right, I had thought this was just testing a useless agg but actually I should change it instead to also have some text and to run the sigText agg over the text field instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++