Skip to content

Commit

Permalink
Aggs fix- don't error if significant text ago run on non-existent fie…
Browse files Browse the repository at this point in the history
…ld. (#70778) (#71066)

Now returns empty results rather than errors when fields don't exist.

Closes #69809
  • Loading branch information
markharwood authored Mar 30, 2021
1 parent 5618be6 commit 1c74f2a
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,10 @@
import org.elasticsearch.search.aggregations.AggregatorFactories;
import org.elasticsearch.search.aggregations.AggregatorFactory;
import org.elasticsearch.search.aggregations.CardinalityUpperBound;
import org.elasticsearch.search.aggregations.InternalAggregation;
import org.elasticsearch.search.aggregations.LeafBucketCollector;
import org.elasticsearch.search.aggregations.LeafBucketCollectorBase;
import org.elasticsearch.search.aggregations.NonCollectingAggregator;
import org.elasticsearch.search.aggregations.bucket.BucketUtils;
import org.elasticsearch.search.aggregations.bucket.terms.IncludeExclude.StringFilter;
import org.elasticsearch.search.aggregations.bucket.terms.MapStringTermsAggregator.CollectConsumer;
Expand Down Expand Up @@ -70,23 +72,34 @@ public SignificantTextAggregatorFactory(String name,
super(name, context, parent, subFactoriesBuilder, metadata);

this.fieldType = context.getFieldType(fieldName);
if (fieldType == null) {
throw new IllegalArgumentException("Field [" + fieldName + "] does not exist, SignificantText " +
"requires an analyzed field");
}
if (supportsAgg(fieldType) == false) {
throw new IllegalArgumentException("Field [" + fieldType.name() + "] has no analyzer, but SignificantText " +
"requires an analyzed field");
if (fieldType != null) {
if (supportsAgg(fieldType) == false) {
throw new IllegalArgumentException("Field [" + fieldType.name() + "] has no analyzer, but SignificantText " +
"requires an analyzed field");
}
String indexedFieldName = fieldType.name();
this.sourceFieldNames = sourceFieldNames == null ? new String[] {indexedFieldName} : sourceFieldNames;
} else {
this.sourceFieldNames = new String[0];
}
String indexedFieldName = fieldType.name();
this.sourceFieldNames = sourceFieldNames == null ? new String[] {indexedFieldName} : sourceFieldNames;

this.includeExclude = includeExclude;
this.backgroundFilter = backgroundFilter;
this.filterDuplicateText = filterDuplicateText;
this.bucketCountThresholds = bucketCountThresholds;
this.significanceHeuristic = significanceHeuristic;
}

protected Aggregator createUnmapped(Aggregator parent, Map<String, Object> metadata) throws IOException {
final InternalAggregation aggregation = new UnmappedSignificantTerms(name, bucketCountThresholds.getRequiredSize(),
bucketCountThresholds.getMinDocCount(), metadata);
return new NonCollectingAggregator(name, context, parent, factories, metadata) {
@Override
public InternalAggregation buildEmptyAggregation() {
return aggregation;
}
};
}

private static boolean supportsAgg(MappedFieldType ft) {
return ft.getTextSearchInfo() != TextSearchInfo.NONE
Expand All @@ -96,6 +109,11 @@ private static boolean supportsAgg(MappedFieldType ft) {
@Override
protected Aggregator createInternal(Aggregator parent, CardinalityUpperBound cardinality, Map<String, Object> metadata)
throws IOException {

if (fieldType == null) {
return createUnmapped(parent, metadata);
}

BucketCountThresholds bucketCountThresholds = new BucketCountThresholds(this.bucketCountThresholds);
if (bucketCountThresholds.getShardSize() == SignificantTextAggregationBuilder.DEFAULT_BUCKET_COUNT_THRESHOLDS.getShardSize()) {
// The user has not made a shardSize selection.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@

import static org.elasticsearch.search.aggregations.AggregationBuilders.sampler;
import static org.elasticsearch.search.aggregations.AggregationBuilders.significantText;
import static org.hamcrest.Matchers.equalTo;

public class SignificantTextAggregatorTests extends AggregatorTestCase {
@Override
Expand Down Expand Up @@ -194,12 +193,9 @@ public void testMissingField() throws IOException {

try (IndexReader reader = DirectoryReader.open(w)) {
IndexSearcher searcher = new IndexSearcher(reader);


IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> searchAndReduce(searcher, new TermQuery(new Term("text", "odd")), aggBuilder, textFieldType));
assertThat(e.getMessage(), equalTo("Field [this_field_does_not_exist] does not exist, SignificantText "
+ "requires an analyzed field"));
InternalSampler sampler = searchAndReduce(searcher, new TermQuery(new Term("text", "odd")), aggBuilder, textFieldType);
SignificantTerms terms = sampler.getAggregations().get("sig_text");
assertTrue(terms.getBuckets().isEmpty());
}
}
}
Expand Down

0 comments on commit 1c74f2a

Please sign in to comment.