From cbcab30090868597e43ab477f18e371ce052cff4 Mon Sep 17 00:00:00 2001 From: val Date: Mon, 8 Aug 2016 14:40:27 +0200 Subject: [PATCH 1/5] Fixes #19768: scripted_metric _agg parameter disappears if params are provided --- .../metrics/scripted/ScriptedMetricAggregatorFactory.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java index f89e99f44b31e..00b03353d7a4b 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java @@ -66,6 +66,8 @@ public Aggregator createInternal(Aggregator parent, boolean collectsFromSingleBu params = deepCopyParams(params, context.searchContext()); } else { params = new HashMap<>(); + } + if (!params.containsKey("_agg")) { params.put("_agg", new HashMap()); } return new ScriptedMetricAggregator(name, insertParams(initScript, params), insertParams(mapScript, params), From 0aa238e89a19c5f4f3fe2a853084d77fffc0d3a1 Mon Sep 17 00:00:00 2001 From: val Date: Wed, 25 Jan 2017 09:25:28 +0100 Subject: [PATCH 2/5] Test case for #19768 --- .../metrics/ScriptedMetricIT.java | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java index 545c10bcb031a..ff5478b06b0d4 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java @@ -351,6 +351,52 @@ public void testMapWithParams() { assertThat(totalCount, equalTo(numDocs)); } + public void testMapWithParamsAndNoImplicitAggMap() { + Map params = new HashMap<>(); + // don't put any _agg map in params + params.put("param1", "12"); + params.put("param2", 1); + + // The _agg hashmap will be available even if not declared in the params map + Script mapScript = new Script(ScriptType.INLINE, CustomScriptPlugin.NAME, "_agg[param1] = param2", params); + + SearchResponse response = client().prepareSearch("idx") + .setQuery(matchAllQuery()) + .addAggregation(scriptedMetric("scripted").params(params).mapScript(mapScript)) + .get(); + assertSearchResponse(response); + assertThat(response.getHits().getTotalHits(), equalTo(numDocs)); + + Aggregation aggregation = response.getAggregations().get("scripted"); + assertThat(aggregation, notNullValue()); + assertThat(aggregation, instanceOf(ScriptedMetric.class)); + ScriptedMetric scriptedMetricAggregation = (ScriptedMetric) aggregation; + assertThat(scriptedMetricAggregation.getName(), equalTo("scripted")); + assertThat(scriptedMetricAggregation.aggregation(), notNullValue()); + assertThat(scriptedMetricAggregation.aggregation(), instanceOf(ArrayList.class)); + List aggregationList = (List) scriptedMetricAggregation.aggregation(); + assertThat(aggregationList.size(), equalTo(getNumShards("idx").numPrimaries)); + long totalCount = 0; + for (Object object : aggregationList) { + assertThat(object, notNullValue()); + assertThat(object, instanceOf(Map.class)); + Map map = (Map) object; + for (Map.Entry entry : map.entrySet()) { + assertThat(entry, notNullValue()); + assertThat(entry.getKey(), notNullValue()); + assertThat(entry.getKey(), instanceOf(String.class)); + assertThat(entry.getValue(), notNullValue()); + assertThat(entry.getValue(), instanceOf(Number.class)); + String stringValue = (String) entry.getKey(); + assertThat(stringValue, equalTo("12")); + Number numberValue = (Number) entry.getValue(); + assertThat(numberValue, equalTo((Number) 1)); + totalCount += numberValue.longValue(); + } + } + assertThat(totalCount, equalTo(numDocs)); + } + public void testInitMapWithParams() { Map varsMap = new HashMap<>(); varsMap.put("multiplier", 1); From 0625ec2f0fd13cd0ef43f3fac2145ca48fb3f140 Mon Sep 17 00:00:00 2001 From: val Date: Wed, 25 Jan 2017 09:43:05 +0100 Subject: [PATCH 3/5] Compare boolean to false instead of negating it --- .../metrics/scripted/ScriptedMetricAggregatorFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java index 3d7d8f0df4009..408123dc78c6f 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java @@ -68,7 +68,7 @@ public Aggregator createInternal(Aggregator parent, boolean collectsFromSingleBu } else { params = new HashMap<>(); } - if (!params.containsKey("_agg")) { + if (params.containsKey("_agg") == false) { params.put("_agg", new HashMap()); } From 883c545e0f24641264fc54217119335b5834c0a1 Mon Sep 17 00:00:00 2001 From: val Date: Wed, 25 Jan 2017 17:47:55 +0100 Subject: [PATCH 4/5] Added mocked script in ScriptedMetricIT --- .../search/aggregations/metrics/ScriptedMetricIT.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java index ff5478b06b0d4..bc474d151f235 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java @@ -91,6 +91,10 @@ protected Map, Object>> pluginScripts() { scripts.put("_agg.add(1)", vars -> aggScript(vars, agg -> ((List) agg).add(1))); + scripts.put("_agg[param1] = param2", vars -> + aggScript(vars, agg -> ((Map) agg).put(XContentMapValues.extractValue("params.param1", vars), + XContentMapValues.extractValue("params.param2", vars)))); + scripts.put("vars.multiplier = 3", vars -> ((Map) vars.get("vars")).put("multiplier", 3)); From 899e2086f969a0b8b1ba245c7e18ed93d4f226cb Mon Sep 17 00:00:00 2001 From: Reese Levine Date: Sat, 28 Oct 2017 12:41:26 -0700 Subject: [PATCH 5/5] Fix test in ScriptedMetricIT for implicit _agg map --- .../search/aggregations/metrics/ScriptedMetricIT.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java index ffdd5956090b9..24d94d5a4643c 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java @@ -360,7 +360,7 @@ public void testMapWithParams() { assertThat(totalCount, equalTo(numDocs)); } - public void testMapWithParamsAndNoImplicitAggMap() { + public void testMapWithParamsAndImplicitAggMap() { Map params = new HashMap<>(); // don't put any _agg map in params params.put("param1", "12"); @@ -385,7 +385,7 @@ public void testMapWithParamsAndNoImplicitAggMap() { assertThat(scriptedMetricAggregation.aggregation(), instanceOf(ArrayList.class)); List aggregationList = (List) scriptedMetricAggregation.aggregation(); assertThat(aggregationList.size(), equalTo(getNumShards("idx").numPrimaries)); - long totalCount = 0; + int numShardsRun = 0; for (Object object : aggregationList) { assertThat(object, notNullValue()); assertThat(object, instanceOf(Map.class)); @@ -400,10 +400,10 @@ public void testMapWithParamsAndNoImplicitAggMap() { assertThat(stringValue, equalTo("12")); Number numberValue = (Number) entry.getValue(); assertThat(numberValue, equalTo((Number) 1)); - totalCount += numberValue.longValue(); + numShardsRun++; } } - assertThat(totalCount, equalTo(numDocs)); + assertThat(numShardsRun, greaterThan(0)); } public void testInitMapWithParams() {