From d9c37ce19591abc2e0c0a776f98b13a0c88ff0e9 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Fri, 17 Feb 2017 16:16:04 -0500 Subject: [PATCH 1/3] Adds unit test for sampler aggregation Relates to #22278 --- .../bucket/DeferringBucketCollector.java | 10 +++ .../aggregations/AggregatorTestCase.java | 13 +-- .../sampler/SamplerAggregatorTests.java | 79 +++++++++++++++++++ 3 files changed, 97 insertions(+), 5 deletions(-) create mode 100644 core/src/test/java/org/elasticsearch/search/aggregations/bucket/sampler/SamplerAggregatorTests.java diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/DeferringBucketCollector.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/DeferringBucketCollector.java index 3c63df2c06a76..bc4eea0ab4a43 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/DeferringBucketCollector.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/DeferringBucketCollector.java @@ -54,6 +54,16 @@ public Aggregator wrap(final Aggregator in) { return new WrappedAggregator(in); } + /** + * Unwrap an aggregator. Used for testing. + */ + public static Aggregator unwrap(Aggregator in) { + if (in instanceof WrappedAggregator) { + return ((WrappedAggregator) in).in; + } + return in; + } + protected class WrappedAggregator extends Aggregator { private Aggregator in; diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java b/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java index aab44c32fcf0d..296427ecc1b02 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java @@ -34,7 +34,6 @@ import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.cache.query.DisabledQueryCache; import org.elasticsearch.index.engine.Engine; -import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.IndexFieldDataCache; import org.elasticsearch.index.fielddata.IndexFieldDataService; import org.elasticsearch.index.mapper.MappedFieldType; @@ -42,6 +41,7 @@ import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.indices.breaker.CircuitBreakerService; import org.elasticsearch.indices.breaker.NoneCircuitBreakerService; +import org.elasticsearch.search.aggregations.bucket.DeferringBucketCollector; import org.elasticsearch.search.fetch.FetchPhase; import org.elasticsearch.search.fetch.subphase.DocValueFieldsFetchSubPhase; import org.elasticsearch.search.fetch.subphase.FetchSourceSubPhase; @@ -110,10 +110,9 @@ public boolean shouldCache(Query query) throws IOException { QueryShardContext queryShardContext = mock(QueryShardContext.class); for (MappedFieldType fieldType : fieldTypes) { - IndexFieldData fieldData = fieldType.fielddataBuilder().build(indexSettings, fieldType, - new IndexFieldDataCache.None(), circuitBreakerService, mock(MapperService.class)); when(queryShardContext.fieldMapper(fieldType.name())).thenReturn(fieldType); - when(queryShardContext.getForField(fieldType)).thenReturn(fieldData); + when(queryShardContext.getForField(fieldType)).then(invocation -> fieldType.fielddataBuilder().build( + indexSettings, fieldType, new IndexFieldDataCache.None(), circuitBreakerService, mock(MapperService.class))); when(searchContext.getQueryShardContext()).thenReturn(queryShardContext); } @@ -126,13 +125,16 @@ protected A search(IndexSe Query query, AggregationBuilder builder, MappedFieldType... fieldTypes) throws IOException { - try (C a = createAggregator(builder, searcher, fieldTypes)) { + C a = createAggregator(builder, searcher, fieldTypes); + try { a.preCollection(); searcher.search(query, a); a.postCollection(); @SuppressWarnings("unchecked") A internalAgg = (A) a.buildAggregation(0L); return internalAgg; + } finally { + closeAgg(a); } } @@ -190,6 +192,7 @@ protected A searchAndReduc } private void closeAgg(Aggregator agg) { + agg = DeferringBucketCollector.unwrap(agg); agg.close(); for (Aggregator sub : ((AggregatorBase) agg).subAggregators) { closeAgg(sub); diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/sampler/SamplerAggregatorTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/sampler/SamplerAggregatorTests.java new file mode 100644 index 0000000000000..5db4caaf65c15 --- /dev/null +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/sampler/SamplerAggregatorTests.java @@ -0,0 +1,79 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.search.aggregations.bucket.sampler; + +import org.apache.lucene.analysis.standard.StandardAnalyzer; +import org.apache.lucene.document.Document; +import org.apache.lucene.document.Field; +import org.apache.lucene.document.SortedNumericDocValuesField; +import org.apache.lucene.index.IndexReader; +import org.apache.lucene.index.RandomIndexWriter; +import org.apache.lucene.index.Term; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.TermQuery; +import org.apache.lucene.store.Directory; +import org.elasticsearch.index.analysis.AnalyzerScope; +import org.elasticsearch.index.analysis.NamedAnalyzer; +import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.index.mapper.NumberFieldMapper; +import org.elasticsearch.index.mapper.TextFieldMapper.TextFieldType; +import org.elasticsearch.search.aggregations.AggregatorTestCase; +import org.elasticsearch.search.aggregations.metrics.min.Min; +import org.elasticsearch.search.aggregations.metrics.min.MinAggregationBuilder; + +import java.io.IOException; + +public class SamplerAggregatorTests extends AggregatorTestCase { + /** + * Uses the sampler aggregation to find the minimum value of a field out of the top 3 scoring documents in a search. + */ + public void testSampler() throws IOException { + TextFieldType textFieldType = new TextFieldType(); + textFieldType.setIndexAnalyzer(new NamedAnalyzer("foo", AnalyzerScope.GLOBAL, new StandardAnalyzer())); + MappedFieldType numericFieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.LONG); + numericFieldType.setName("int"); + + try (Directory dir = newDirectory(); + RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { + for (long value : new long[] {7, 3, -10, -6, 5, 50}) { + Document doc = new Document(); + StringBuilder text = new StringBuilder(); + for (int i = 0; i < value; i++) { + text.append("good "); + } + doc.add(new Field("text", text.toString(), textFieldType)); + doc.add(new SortedNumericDocValuesField("int", value)); + w.addDocument(doc); + } + + SamplerAggregationBuilder aggBuilder = new SamplerAggregationBuilder("sampler") + .shardSize(3) + .subAggregation(new MinAggregationBuilder("min") + .field("int")); + try (IndexReader reader = w.getReader()) { + IndexSearcher searcher = new IndexSearcher(reader); + Sampler sampler = searchAndReduce(searcher, new TermQuery(new Term("text", "good")), aggBuilder, textFieldType, + numericFieldType); + Min min = sampler.getAggregations().get("min"); + assertEquals(5.0, min.getValue(), Double.MIN_NORMAL); + } + } + } +} From 0dee1f85e6d4ab7280757833ae4028b5b1b1dc88 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 21 Feb 2017 10:31:42 -0500 Subject: [PATCH 2/3] Remove closeAgg --- .../bucket/DeferringBucketCollector.java | 10 ----- .../aggregations/AggregatorTestCase.java | 37 +++++++++---------- .../sampler/SamplerAggregatorTests.java | 2 +- 3 files changed, 19 insertions(+), 30 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/DeferringBucketCollector.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/DeferringBucketCollector.java index bc4eea0ab4a43..3c63df2c06a76 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/DeferringBucketCollector.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/DeferringBucketCollector.java @@ -54,16 +54,6 @@ public Aggregator wrap(final Aggregator in) { return new WrappedAggregator(in); } - /** - * Unwrap an aggregator. Used for testing. - */ - public static Aggregator unwrap(Aggregator in) { - if (in instanceof WrappedAggregator) { - return ((WrappedAggregator) in).in; - } - return in; - } - protected class WrappedAggregator extends Aggregator { private Aggregator in; diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java b/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java index 296427ecc1b02..2690aeb4507ee 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java @@ -29,6 +29,8 @@ import org.apache.lucene.search.Weight; import org.elasticsearch.Version; import org.elasticsearch.cluster.metadata.IndexMetaData; +import org.elasticsearch.common.lease.Releasable; +import org.elasticsearch.common.lease.Releasables; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.MockBigArrays; import org.elasticsearch.index.IndexSettings; @@ -41,7 +43,6 @@ import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.indices.breaker.CircuitBreakerService; import org.elasticsearch.indices.breaker.NoneCircuitBreakerService; -import org.elasticsearch.search.aggregations.bucket.DeferringBucketCollector; import org.elasticsearch.search.fetch.FetchPhase; import org.elasticsearch.search.fetch.subphase.DocValueFieldsFetchSubPhase; import org.elasticsearch.search.fetch.subphase.FetchSourceSubPhase; @@ -56,6 +57,8 @@ import java.util.Collections; import java.util.List; +import static org.mockito.Matchers.anyObject; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -65,6 +68,8 @@ * {@link AggregationBuilder} instance. */ public abstract class AggregatorTestCase extends ESTestCase { + private List releasables = new ArrayList<>(); + protected A createAggregator(B aggregationBuilder, IndexSearcher indexSearcher, MappedFieldType... fieldTypes) throws IOException { @@ -99,6 +104,10 @@ public boolean shouldCache(Query query) throws IOException { when(searchContext.bigArrays()).thenReturn(new MockBigArrays(Settings.EMPTY, circuitBreakerService)); when(searchContext.fetchPhase()) .thenReturn(new FetchPhase(Arrays.asList(new FetchSourceSubPhase(), new DocValueFieldsFetchSubPhase()))); + doAnswer(invocation -> { + releasables.add((Releasable) invocation.getArguments()[0]); + return null; + }).when(searchContext).addReleasable(anyObject(), anyObject()); // TODO: now just needed for top_hits, this will need to be revised for other agg unit tests: MapperService mapperService = mock(MapperService.class); @@ -134,7 +143,8 @@ protected A search(IndexSe A internalAgg = (A) a.buildAggregation(0L); return internalAgg; } finally { - closeAgg(a); + Releasables.close(releasables); + releasables.clear(); } } @@ -170,14 +180,10 @@ protected A searchAndReduc try { for (ShardSearcher subSearcher : subSearchers) { C a = createAggregator(builder, subSearcher, fieldTypes); - try { - a.preCollection(); - subSearcher.search(weight, a); - a.postCollection(); - aggs.add(a.buildAggregation(0L)); - } finally { - closeAgg(a); - } + a.preCollection(); + subSearcher.search(weight, a); + a.postCollection(); + aggs.add(a.buildAggregation(0L)); } if (aggs.isEmpty()) { return null; @@ -187,15 +193,8 @@ protected A searchAndReduc return internalAgg; } } finally { - closeAgg(root); - } - } - - private void closeAgg(Aggregator agg) { - agg = DeferringBucketCollector.unwrap(agg); - agg.close(); - for (Aggregator sub : ((AggregatorBase) agg).subAggregators) { - closeAgg(sub); + Releasables.close(releasables); + releasables.clear(); } } diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/sampler/SamplerAggregatorTests.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/sampler/SamplerAggregatorTests.java index 5db4caaf65c15..3f7a458ae753f 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/sampler/SamplerAggregatorTests.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/sampler/SamplerAggregatorTests.java @@ -72,7 +72,7 @@ public void testSampler() throws IOException { Sampler sampler = searchAndReduce(searcher, new TermQuery(new Term("text", "good")), aggBuilder, textFieldType, numericFieldType); Min min = sampler.getAggregations().get("min"); - assertEquals(5.0, min.getValue(), Double.MIN_NORMAL); + assertEquals(5.0, min.getValue(), 0); } } } From 74c33823abec758760d65144dcc231aeeae3e46e Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 21 Feb 2017 10:43:29 -0500 Subject: [PATCH 3/3] Comment --- .../elasticsearch/search/aggregations/AggregatorTestCase.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java b/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java index 2690aeb4507ee..2af21f4568940 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java @@ -105,6 +105,8 @@ public boolean shouldCache(Query query) throws IOException { when(searchContext.fetchPhase()) .thenReturn(new FetchPhase(Arrays.asList(new FetchSourceSubPhase(), new DocValueFieldsFetchSubPhase()))); doAnswer(invocation -> { + /* Store the releasables so we can release them at the end of the test case. This is important because aggregations don't + * close their sub-aggregations. This is fairly similar to what the production code does. */ releasables.add((Releasable) invocation.getArguments()[0]); return null; }).when(searchContext).addReleasable(anyObject(), anyObject());