From 0aff5a30c5dbad9f476be14f34b81e2d1991bb0f Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Mon, 24 Sep 2018 16:09:22 +0200 Subject: [PATCH 1/4] Check self references in metric agg after last doc collection (#33593) --- .../aggregations/metrics/ScriptedMetricAggregator.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java index 345b21d03887e..701b0c052daf3 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java @@ -68,7 +68,10 @@ public ScoreMode scoreMode() { public LeafBucketCollector getLeafCollector(LeafReaderContext ctx, final LeafBucketCollector sub) throws IOException { final ScriptedMetricAggContexts.MapScript leafMapScript = mapScript.newInstance(ctx); + final int numDocs = ctx.reader().numDocs(); return new LeafBucketCollectorBase(sub, leafMapScript) { + private long docCount = 0; + @Override public void setScorer(Scorable scorer) throws IOException { leafMapScript.setScorer(scorer); @@ -80,7 +83,11 @@ public void collect(int doc, long bucket) throws IOException { leafMapScript.setDocument(doc); leafMapScript.execute(); - CollectionUtils.ensureNoSelfReferences(aggState, "Scripted metric aggs map script"); + docCount++; + + if (docCount == numDocs) { + CollectionUtils.ensureNoSelfReferences(aggState, "Scripted metric aggs map script"); + } } }; } From cb6e3079c6eabd99c908a26c1afca685bfbd3d01 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Wed, 26 Sep 2018 09:28:32 +0200 Subject: [PATCH 2/4] Revert 0aff5a30c5dbad9f476be14f34b81e2d1991bb0f (#33593) --- .../aggregations/metrics/ScriptedMetricAggregator.java | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java index 701b0c052daf3..345b21d03887e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java @@ -68,10 +68,7 @@ public ScoreMode scoreMode() { public LeafBucketCollector getLeafCollector(LeafReaderContext ctx, final LeafBucketCollector sub) throws IOException { final ScriptedMetricAggContexts.MapScript leafMapScript = mapScript.newInstance(ctx); - final int numDocs = ctx.reader().numDocs(); return new LeafBucketCollectorBase(sub, leafMapScript) { - private long docCount = 0; - @Override public void setScorer(Scorable scorer) throws IOException { leafMapScript.setScorer(scorer); @@ -83,11 +80,7 @@ public void collect(int doc, long bucket) throws IOException { leafMapScript.setDocument(doc); leafMapScript.execute(); - docCount++; - - if (docCount == numDocs) { - CollectionUtils.ensureNoSelfReferences(aggState, "Scripted metric aggs map script"); - } + CollectionUtils.ensureNoSelfReferences(aggState, "Scripted metric aggs map script"); } }; } From cc10fec2ef9a6726f8df57b0385861f31d35321a Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Wed, 26 Sep 2018 10:44:01 +0200 Subject: [PATCH 3/4] Check self refs in metric agg only once in post collection hook (#33593) --- .../metrics/ScriptedMetricAggregator.java | 11 +++++- .../ScriptedMetricAggregatorTests.java | 35 +++++++++++++++++-- 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java index 345b21d03887e..27bc86aeb911a 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java @@ -80,7 +80,6 @@ public void collect(int doc, long bucket) throws IOException { leafMapScript.setDocument(doc); leafMapScript.execute(); - CollectionUtils.ensureNoSelfReferences(aggState, "Scripted metric aggs map script"); } }; } @@ -103,4 +102,14 @@ public InternalAggregation buildEmptyAggregation() { return new InternalScriptedMetric(name, null, reduceScript, pipelineAggregators(), metaData()); } + @Override + protected void doPostCollection() throws IOException { + ensureNoSelfReferencesInAggState(); + + super.doPostCollection(); + } + + void ensureNoSelfReferencesInAggState() { + CollectionUtils.ensureNoSelfReferences(aggState, "Scripted metric aggs map script"); + } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregatorTests.java index 56b8938b6e54b..39ec9cd2d2183 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregatorTests.java @@ -23,9 +23,13 @@ import org.apache.lucene.index.DirectoryReader; import org.apache.lucene.index.IndexReader; import org.apache.lucene.index.RandomIndexWriter; +import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.MatchAllDocsQuery; +import org.apache.lucene.search.Query; import org.apache.lucene.store.Directory; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.IndexSettings; +import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.script.MockScriptEngine; @@ -34,9 +38,11 @@ import org.elasticsearch.script.ScriptModule; import org.elasticsearch.script.ScriptService; import org.elasticsearch.script.ScriptType; +import org.elasticsearch.search.aggregations.AggregationBuilder; +import org.elasticsearch.search.aggregations.Aggregator; import org.elasticsearch.search.aggregations.AggregatorTestCase; -import org.elasticsearch.search.aggregations.metrics.ScriptedMetric; -import org.elasticsearch.search.aggregations.metrics.ScriptedMetricAggregationBuilder; +import org.elasticsearch.search.aggregations.MultiBucketConsumerService; +import org.junit.After; import org.junit.BeforeClass; import java.io.IOException; @@ -48,6 +54,8 @@ import java.util.function.Function; import static java.util.Collections.singleton; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; public class ScriptedMetricAggregatorTests extends AggregatorTestCase { @@ -81,6 +89,8 @@ public class ScriptedMetricAggregatorTests extends AggregatorTestCase { private static final Map, Object>> SCRIPTS = new HashMap<>(); + private ScriptedMetricAggregator aggregator = null; + @BeforeClass @SuppressWarnings("unchecked") public static void initMockScripts() { @@ -154,6 +164,15 @@ public static void initMockScripts() { }); } + @After + public void ensureNoSelfReferencesInAggState() { + if (aggregator != null) { + verify(aggregator).ensureNoSelfReferencesInAggState(); + + aggregator = null; + } + } + @SuppressWarnings("unchecked") public void testNoDocs() throws IOException { try (Directory directory = newDirectory()) { @@ -338,6 +357,18 @@ public void testSelfReferencingAggStateAfterCombine() throws IOException { } } + @Override + @SuppressWarnings("unchecked") + protected A createAggregator(Query query, + AggregationBuilder aggregationBuilder, + IndexSearcher indexSearcher, + IndexSettings indexSettings, + MultiBucketConsumerService.MultiBucketConsumer bucketConsumer, + MappedFieldType... fieldTypes) throws IOException { + aggregator = spy(super.createAggregator(query, aggregationBuilder, indexSearcher, indexSettings, bucketConsumer, fieldTypes)); + return (A) aggregator; + } + /** * We cannot use Mockito for mocking QueryShardContext in this case because * script-related methods (e.g. QueryShardContext#getLazyExecutableScript) From 671d6c6a3928e9c692c121faca17e08ffc02a4a0 Mon Sep 17 00:00:00 2001 From: Christophe Bismuth Date: Wed, 24 Oct 2018 15:59:06 +0200 Subject: [PATCH 4/4] Remove unnecessary mocking (#33593) --- .../metrics/ScriptedMetricAggregator.java | 6 +--- .../ScriptedMetricAggregatorTests.java | 33 ------------------- 2 files changed, 1 insertion(+), 38 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java index 27bc86aeb911a..da936a76ee1ca 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregator.java @@ -104,12 +104,8 @@ public InternalAggregation buildEmptyAggregation() { @Override protected void doPostCollection() throws IOException { - ensureNoSelfReferencesInAggState(); + CollectionUtils.ensureNoSelfReferences(aggState, "Scripted metric aggs map script"); super.doPostCollection(); } - - void ensureNoSelfReferencesInAggState() { - CollectionUtils.ensureNoSelfReferences(aggState, "Scripted metric aggs map script"); - } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregatorTests.java index e535455b8b8bf..05115a03e300f 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricAggregatorTests.java @@ -23,13 +23,9 @@ import org.apache.lucene.index.DirectoryReader; import org.apache.lucene.index.IndexReader; import org.apache.lucene.index.RandomIndexWriter; -import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.MatchAllDocsQuery; -import org.apache.lucene.search.Query; import org.apache.lucene.store.Directory; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.index.IndexSettings; -import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.script.MockScriptEngine; @@ -38,11 +34,7 @@ import org.elasticsearch.script.ScriptModule; import org.elasticsearch.script.ScriptService; import org.elasticsearch.script.ScriptType; -import org.elasticsearch.search.aggregations.AggregationBuilder; -import org.elasticsearch.search.aggregations.Aggregator; import org.elasticsearch.search.aggregations.AggregatorTestCase; -import org.elasticsearch.search.aggregations.MultiBucketConsumerService; -import org.junit.After; import org.junit.BeforeClass; import java.io.IOException; @@ -54,8 +46,6 @@ import java.util.function.Function; import static java.util.Collections.singleton; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.verify; public class ScriptedMetricAggregatorTests extends AggregatorTestCase { @@ -93,8 +83,6 @@ public class ScriptedMetricAggregatorTests extends AggregatorTestCase { private static final Map, Object>> SCRIPTS = new HashMap<>(); - private ScriptedMetricAggregator aggregator = null; - @BeforeClass @SuppressWarnings("unchecked") public static void initMockScripts() { @@ -176,15 +164,6 @@ public static void initMockScripts() { }); } - @After - public void ensureNoSelfReferencesInAggState() { - if (aggregator != null) { - verify(aggregator).ensureNoSelfReferencesInAggState(); - - aggregator = null; - } - } - @SuppressWarnings("unchecked") public void testNoDocs() throws IOException { try (Directory directory = newDirectory()) { @@ -385,18 +364,6 @@ public void testSelfReferencingAggStateAfterCombine() throws IOException { } } - @Override - @SuppressWarnings("unchecked") - protected A createAggregator(Query query, - AggregationBuilder aggregationBuilder, - IndexSearcher indexSearcher, - IndexSettings indexSettings, - MultiBucketConsumerService.MultiBucketConsumer bucketConsumer, - MappedFieldType... fieldTypes) throws IOException { - aggregator = spy(super.createAggregator(query, aggregationBuilder, indexSearcher, indexSettings, bucketConsumer, fieldTypes)); - return (A) aggregator; - } - /** * We cannot use Mockito for mocking QueryShardContext in this case because * script-related methods (e.g. QueryShardContext#getLazyExecutableScript)