Remove support for deprecated params._agg/_aggs for scripted metric aggregations (#32979)

This commit is contained in:
Jonathan Little 2018-08-28 01:27:43 -07:00 committed by Colin Goodheart-Smithe
parent 19ef41ee82
commit 9d92a87ae6
11 changed files with 20 additions and 366 deletions

View File

@ -802,8 +802,6 @@ class BuildPlugin implements Plugin<Project> {
systemProperty 'tests.task', path
systemProperty 'tests.security.manager', 'true'
systemProperty 'jna.nosys', 'true'
// TODO: remove this deprecation compatibility setting for 7.0
systemProperty 'es.aggregations.enable_scripted_metric_agg_param', 'false'
systemProperty 'compiler.java', project.ext.compilerJavaVersion.getMajorVersion()
if (project.ext.inFipsJvm) {
systemProperty 'runtime.java', project.ext.runtimeJavaVersion.getMajorVersion() + "FIPS"

View File

@ -41,9 +41,6 @@ integTestCluster {
// TODO: remove this for 7.0, this exists to allow the doc examples in 6.x to continue using the defaults
systemProperty 'es.scripting.use_java_time', 'false'
systemProperty 'es.scripting.update.ctx_in_params', 'false'
// TODO: remove this deprecation compatibility setting for 7.0
systemProperty 'es.aggregations.enable_scripted_metric_agg_param', 'false'
}
// remove when https://github.com/elastic/elasticsearch/issues/31305 is fixed

View File

@ -21,5 +21,3 @@ has been removed. `missing_bucket` should be used instead.
The object used to share aggregation state between the scripts in a Scripted Metric
Aggregation is now a variable called `state` available in the script context, rather than
being provided via the `params` object as `params._agg`.
The old `params._agg` variable is still available as well.

View File

@ -346,16 +346,3 @@ if (isEclipse == false || project.path == ":server-tests") {
check.dependsOn integTest
integTest.mustRunAfter test
}
// TODO: remove these compatibility tests in 7.0
additionalTest('testScriptedMetricAggParamsV6Compatibility') {
include '**/ScriptedMetricAggregatorAggStateV6CompatTests.class'
include '**/InternalScriptedMetricAggStateV6CompatTests.class'
systemProperty 'es.aggregations.enable_scripted_metric_agg_param', 'true'
}
test {
// these are tested explicitly in separate test tasks
exclude '**/ScriptedMetricAggregatorAggStateV6CompatTests.class'
exclude '**/InternalScriptedMetricAggStateV6CompatTests.class'
}

View File

@ -22,8 +22,6 @@ package org.elasticsearch.script;
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.search.Scorer;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.index.fielddata.ScriptDocValues;
import org.elasticsearch.search.lookup.LeafSearchLookup;
import org.elasticsearch.search.lookup.SearchLookup;
@ -33,30 +31,11 @@ import java.util.List;
import java.util.Map;
public class ScriptedMetricAggContexts {
private static final DeprecationLogger DEPRECATION_LOGGER =
new DeprecationLogger(Loggers.getLogger(ScriptedMetricAggContexts.class));
// Public for access from tests
public static final String AGG_PARAM_DEPRECATION_WARNING =
"params._agg/_aggs for scripted metric aggregations are deprecated, use state/states (not in params) instead. " +
"Use -Des.aggregations.enable_scripted_metric_agg_param=false to disable.";
public static boolean deprecatedAggParamEnabled() {
boolean enabled = Boolean.parseBoolean(
System.getProperty("es.aggregations.enable_scripted_metric_agg_param", "true"));
if (enabled) {
DEPRECATION_LOGGER.deprecatedAndMaybeLog("enable_scripted_metric_agg_param", AGG_PARAM_DEPRECATION_WARNING);
}
return enabled;
}
private abstract static class ParamsAndStateBase {
private final Map<String, Object> params;
private final Object state;
private final Map<String, Object> state;
ParamsAndStateBase(Map<String, Object> params, Object state) {
ParamsAndStateBase(Map<String, Object> params, Map<String, Object> state) {
this.params = params;
this.state = state;
}
@ -71,14 +50,14 @@ public class ScriptedMetricAggContexts {
}
public abstract static class InitScript extends ParamsAndStateBase {
public InitScript(Map<String, Object> params, Object state) {
public InitScript(Map<String, Object> params, Map<String, Object> state) {
super(params, state);
}
public abstract void execute();
public interface Factory {
InitScript newInstance(Map<String, Object> params, Object state);
InitScript newInstance(Map<String, Object> params, Map<String, Object> state);
}
public static String[] PARAMETERS = {};
@ -89,7 +68,7 @@ public class ScriptedMetricAggContexts {
private final LeafSearchLookup leafLookup;
private Scorer scorer;
public MapScript(Map<String, Object> params, Object state, SearchLookup lookup, LeafReaderContext leafContext) {
public MapScript(Map<String, Object> params, Map<String, Object> state, SearchLookup lookup, LeafReaderContext leafContext) {
super(params, state);
this.leafLookup = leafContext == null ? null : lookup.getLeafSearchLookup(leafContext);
@ -131,7 +110,7 @@ public class ScriptedMetricAggContexts {
}
public interface Factory {
LeafFactory newFactory(Map<String, Object> params, Object state, SearchLookup lookup);
LeafFactory newFactory(Map<String, Object> params, Map<String, Object> state, SearchLookup lookup);
}
public static String[] PARAMETERS = new String[] {};
@ -139,14 +118,14 @@ public class ScriptedMetricAggContexts {
}
public abstract static class CombineScript extends ParamsAndStateBase {
public CombineScript(Map<String, Object> params, Object state) {
public CombineScript(Map<String, Object> params, Map<String, Object> state) {
super(params, state);
}
public abstract Object execute();
public interface Factory {
CombineScript newInstance(Map<String, Object> params, Object state);
CombineScript newInstance(Map<String, Object> params, Map<String, Object> state);
}
public static String[] PARAMETERS = {};

View File

@ -95,11 +95,6 @@ public class InternalScriptedMetric extends InternalAggregation implements Scrip
params.putAll(firstAggregation.reduceScript.getParams());
}
// Add _aggs to params map for backwards compatibility (redundant with a context variable on the ReduceScript created below).
if (ScriptedMetricAggContexts.deprecatedAggParamEnabled()) {
params.put("_aggs", aggregationObjects);
}
ScriptedMetricAggContexts.ReduceScript.Factory factory = reduceContext.scriptService().compile(
firstAggregation.reduceScript, ScriptedMetricAggContexts.ReduceScript.CONTEXT);
ScriptedMetricAggContexts.ReduceScript script = factory.newInstance(params, aggregationObjects);

View File

@ -41,10 +41,10 @@ public class ScriptedMetricAggregator extends MetricsAggregator {
private final ScriptedMetricAggContexts.MapScript.LeafFactory mapScript;
private final ScriptedMetricAggContexts.CombineScript combineScript;
private final Script reduceScript;
private Object aggState;
private Map<String, Object> aggState;
protected ScriptedMetricAggregator(String name, ScriptedMetricAggContexts.MapScript.LeafFactory mapScript, ScriptedMetricAggContexts.CombineScript combineScript,
Script reduceScript, Object aggState, SearchContext context, Aggregator parent,
Script reduceScript, Map<String, Object> aggState, SearchContext context, Aggregator parent,
List<PipelineAggregator> pipelineAggregators, Map<String, Object> metaData)
throws IOException {
super(name, context, parent, pipelineAggregators, metaData);

View File

@ -80,20 +80,7 @@ public class ScriptedMetricAggregatorFactory extends AggregatorFactory<ScriptedM
aggParams = new HashMap<>();
}
// Add _agg to params map for backwards compatibility (redundant with context variables on the scripts created below).
// When this is removed, aggState (as passed to ScriptedMetricAggregator) can be changed to Map<String, Object>, since
// it won't be possible to completely replace it with another type as is possible when it's an entry in params.
Object aggState = new HashMap<String, Object>();
if (ScriptedMetricAggContexts.deprecatedAggParamEnabled()) {
if (aggParams.containsKey("_agg") == false) {
// Add _agg if it wasn't added manually
aggParams.put("_agg", aggState);
} else {
// If it was added manually, also use it for the agg context variable to reduce the likelihood of
// weird behavior due to multiple different variables.
aggState = aggParams.get("_agg");
}
}
Map<String, Object> aggState = new HashMap<String, Object>();
final ScriptedMetricAggContexts.InitScript initScript = this.initScript.newInstance(
mergeParams(aggParams, initScriptParams), aggState);

View File

@ -1,109 +0,0 @@
/*
* 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.metrics.scripted;
import org.elasticsearch.common.io.stream.Writeable.Reader;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.script.MockScriptEngine;
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptedMetricAggContexts;
import org.elasticsearch.script.ScriptEngine;
import org.elasticsearch.script.ScriptModule;
import org.elasticsearch.script.ScriptService;
import org.elasticsearch.script.ScriptType;
import org.elasticsearch.search.aggregations.Aggregation.CommonFields;
import org.elasticsearch.search.aggregations.ParsedAggregation;
import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
import org.elasticsearch.test.InternalAggregationTestCase;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.function.Function;
import java.util.function.Predicate;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.sameInstance;
/**
* This test verifies that the _aggs param is added correctly when the system property
* "es.aggregations.enable_scripted_metric_agg_param" is set to true.
*/
public class InternalScriptedMetricAggStateV6CompatTests extends InternalAggregationTestCase<InternalScriptedMetric> {
private static final String REDUCE_SCRIPT_NAME = "reduceScript";
@Override
protected InternalScriptedMetric createTestInstance(String name, List<PipelineAggregator> pipelineAggregators,
Map<String, Object> metaData) {
Script reduceScript = new Script(ScriptType.INLINE, MockScriptEngine.NAME, REDUCE_SCRIPT_NAME, Collections.emptyMap());
return new InternalScriptedMetric(name, "agg value", reduceScript, pipelineAggregators, metaData);
}
/**
* Mock of the script service. The script that is run looks at the
* "_aggs" parameter to verify that it was put in place by InternalScriptedMetric.
*/
@Override
protected ScriptService mockScriptService() {
Function<Map<String, Object>, Object> script = params -> {
Object aggs = params.get("_aggs");
Object states = params.get("states");
assertThat(aggs, instanceOf(List.class));
assertThat(aggs, sameInstance(states));
return aggs;
};
@SuppressWarnings("unchecked")
MockScriptEngine scriptEngine = new MockScriptEngine(MockScriptEngine.NAME,
Collections.singletonMap(REDUCE_SCRIPT_NAME, script));
Map<String, ScriptEngine> engines = Collections.singletonMap(scriptEngine.getType(), scriptEngine);
return new ScriptService(Settings.EMPTY, engines, ScriptModule.CORE_CONTEXTS);
}
@Override
protected void assertReduced(InternalScriptedMetric reduced, List<InternalScriptedMetric> inputs) {
assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
}
@Override
protected Reader<InternalScriptedMetric> instanceReader() {
return InternalScriptedMetric::new;
}
@Override
protected void assertFromXContent(InternalScriptedMetric aggregation, ParsedAggregation parsedAggregation) {}
@Override
protected Predicate<String> excludePathsFromXContentInsertion() {
return path -> path.contains(CommonFields.VALUE.getPreferredName());
}
@Override
protected InternalScriptedMetric mutateInstance(InternalScriptedMetric instance) {
String name = instance.getName();
Object value = instance.aggregation();
Script reduceScript = instance.reduceScript;
List<PipelineAggregator> pipelineAggregators = instance.pipelineAggregators();
Map<String, Object> metaData = instance.getMetaData();
return new InternalScriptedMetric(name + randomAlphaOfLength(5), value, reduceScript, pipelineAggregators,
metaData);
}
}

View File

@ -1,180 +0,0 @@
/*
* 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.metrics.scripted;
import org.apache.lucene.document.SortedNumericDocValuesField;
import org.apache.lucene.index.DirectoryReader;
import org.apache.lucene.index.IndexReader;
import org.apache.lucene.index.RandomIndexWriter;
import org.apache.lucene.search.MatchAllDocsQuery;
import org.apache.lucene.store.Directory;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.mapper.MapperService;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.script.MockScriptEngine;
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptedMetricAggContexts;
import org.elasticsearch.script.ScriptEngine;
import org.elasticsearch.script.ScriptModule;
import org.elasticsearch.script.ScriptService;
import org.elasticsearch.script.ScriptType;
import org.elasticsearch.search.aggregations.AggregatorTestCase;
import org.junit.BeforeClass;
import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Function;
import static java.util.Collections.singleton;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.sameInstance;
/**
* This test verifies that the _agg param is added correctly when the system property
* "es.aggregations.enable_scripted_metric_agg_param" is set to true.
*/
public class ScriptedMetricAggregatorAggStateV6CompatTests extends AggregatorTestCase {
private static final String AGG_NAME = "scriptedMetric";
private static final Script INIT_SCRIPT = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "initScript", Collections.emptyMap());
private static final Script MAP_SCRIPT = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "mapScript", Collections.emptyMap());
private static final Script COMBINE_SCRIPT = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "combineScript",
Collections.emptyMap());
private static final Script INIT_SCRIPT_EXPLICIT_AGG = new Script(ScriptType.INLINE, MockScriptEngine.NAME,
"initScriptExplicitAgg", Collections.emptyMap());
private static final Script MAP_SCRIPT_EXPLICIT_AGG = new Script(ScriptType.INLINE, MockScriptEngine.NAME,
"mapScriptExplicitAgg", Collections.emptyMap());
private static final Script COMBINE_SCRIPT_EXPLICIT_AGG = new Script(ScriptType.INLINE, MockScriptEngine.NAME,
"combineScriptExplicitAgg", Collections.emptyMap());
private static final String EXPLICIT_AGG_OBJECT = "Explicit agg object";
private static final Map<String, Function<Map<String, Object>, Object>> SCRIPTS = new HashMap<>();
@BeforeClass
@SuppressWarnings("unchecked")
public static void initMockScripts() {
// If _agg is provided implicitly, it should be the same objects as "state" from the context.
SCRIPTS.put("initScript", params -> {
Object agg = params.get("_agg");
Object state = params.get("state");
assertThat(agg, instanceOf(Map.class));
assertThat(agg, sameInstance(state));
return agg;
});
SCRIPTS.put("mapScript", params -> {
Object agg = params.get("_agg");
Object state = params.get("state");
assertThat(agg, instanceOf(Map.class));
assertThat(agg, sameInstance(state));
return agg;
});
SCRIPTS.put("combineScript", params -> {
Object agg = params.get("_agg");
Object state = params.get("state");
assertThat(agg, instanceOf(Map.class));
assertThat(agg, sameInstance(state));
return agg;
});
SCRIPTS.put("initScriptExplicitAgg", params -> {
Object agg = params.get("_agg");
assertThat(agg, equalTo(EXPLICIT_AGG_OBJECT));
return agg;
});
SCRIPTS.put("mapScriptExplicitAgg", params -> {
Object agg = params.get("_agg");
assertThat(agg, equalTo(EXPLICIT_AGG_OBJECT));
return agg;
});
SCRIPTS.put("combineScriptExplicitAgg", params -> {
Object agg = params.get("_agg");
assertThat(agg, equalTo(EXPLICIT_AGG_OBJECT));
return agg;
});
}
/**
* Test that the _agg param is implicitly added
*/
public void testWithImplicitAggParam() throws IOException {
try (Directory directory = newDirectory()) {
Integer numDocs = 10;
try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
for (int i = 0; i < numDocs; i++) {
indexWriter.addDocument(singleton(new SortedNumericDocValuesField("number", i)));
}
}
try (IndexReader indexReader = DirectoryReader.open(directory)) {
ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME);
aggregationBuilder.initScript(INIT_SCRIPT).mapScript(MAP_SCRIPT).combineScript(COMBINE_SCRIPT);
search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder);
}
}
assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
}
/**
* Test that an explicitly added _agg param is honored
*/
public void testWithExplicitAggParam() throws IOException {
try (Directory directory = newDirectory()) {
Integer numDocs = 10;
try (RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
for (int i = 0; i < numDocs; i++) {
indexWriter.addDocument(singleton(new SortedNumericDocValuesField("number", i)));
}
}
Map<String, Object> aggParams = new HashMap<>();
aggParams.put("_agg", EXPLICIT_AGG_OBJECT);
try (IndexReader indexReader = DirectoryReader.open(directory)) {
ScriptedMetricAggregationBuilder aggregationBuilder = new ScriptedMetricAggregationBuilder(AGG_NAME);
aggregationBuilder
.params(aggParams)
.initScript(INIT_SCRIPT_EXPLICIT_AGG)
.mapScript(MAP_SCRIPT_EXPLICIT_AGG)
.combineScript(COMBINE_SCRIPT_EXPLICIT_AGG);
search(newSearcher(indexReader, true, true), new MatchAllDocsQuery(), aggregationBuilder);
}
}
assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
}
/**
* We cannot use Mockito for mocking QueryShardContext in this case because
* script-related methods (e.g. QueryShardContext#getLazyExecutableScript)
* is final and cannot be mocked
*/
@Override
protected QueryShardContext queryShardContextMock(MapperService mapperService) {
MockScriptEngine scriptEngine = new MockScriptEngine(MockScriptEngine.NAME, SCRIPTS);
Map<String, ScriptEngine> engines = Collections.singletonMap(scriptEngine.getType(), scriptEngine);
ScriptService scriptService = new ScriptService(Settings.EMPTY, engines, ScriptModule.CORE_CONTEXTS);
return new QueryShardContext(0, mapperService.getIndexSettings(), null, null, mapperService, null, scriptService,
xContentRegistry(), writableRegistry(), null, null, System::currentTimeMillis, null);
}
}

View File

@ -242,16 +242,18 @@ public class MockScriptEngine implements ScriptEngine {
return new MockMovingFunctionScript();
}
public ScriptedMetricAggContexts.InitScript createMetricAggInitScript(Map<String, Object> params, Object state) {
public ScriptedMetricAggContexts.InitScript createMetricAggInitScript(Map<String, Object> params, Map<String, Object> state) {
return new MockMetricAggInitScript(params, state, script != null ? script : ctx -> 42d);
}
public ScriptedMetricAggContexts.MapScript.LeafFactory createMetricAggMapScript(Map<String, Object> params, Object state,
public ScriptedMetricAggContexts.MapScript.LeafFactory createMetricAggMapScript(Map<String, Object> params,
Map<String, Object> state,
SearchLookup lookup) {
return new MockMetricAggMapScript(params, state, lookup, script != null ? script : ctx -> 42d);
}
public ScriptedMetricAggContexts.CombineScript createMetricAggCombineScript(Map<String, Object> params, Object state) {
public ScriptedMetricAggContexts.CombineScript createMetricAggCombineScript(Map<String, Object> params,
Map<String, Object> state) {
return new MockMetricAggCombineScript(params, state, script != null ? script : ctx -> 42d);
}
@ -415,7 +417,7 @@ public class MockScriptEngine implements ScriptEngine {
public static class MockMetricAggInitScript extends ScriptedMetricAggContexts.InitScript {
private final Function<Map<String, Object>, Object> script;
MockMetricAggInitScript(Map<String, Object> params, Object state,
MockMetricAggInitScript(Map<String, Object> params, Map<String, Object> state,
Function<Map<String, Object>, Object> script) {
super(params, state);
this.script = script;
@ -436,11 +438,11 @@ public class MockScriptEngine implements ScriptEngine {
public static class MockMetricAggMapScript implements ScriptedMetricAggContexts.MapScript.LeafFactory {
private final Map<String, Object> params;
private final Object state;
private final Map<String, Object> state;
private final SearchLookup lookup;
private final Function<Map<String, Object>, Object> script;
MockMetricAggMapScript(Map<String, Object> params, Object state, SearchLookup lookup,
MockMetricAggMapScript(Map<String, Object> params, Map<String, Object> state, SearchLookup lookup,
Function<Map<String, Object>, Object> script) {
this.params = params;
this.state = state;
@ -473,7 +475,7 @@ public class MockScriptEngine implements ScriptEngine {
public static class MockMetricAggCombineScript extends ScriptedMetricAggContexts.CombineScript {
private final Function<Map<String, Object>, Object> script;
MockMetricAggCombineScript(Map<String, Object> params, Object state,
MockMetricAggCombineScript(Map<String, Object> params, Map<String, Object> state,
Function<Map<String, Object>, Object> script) {
super(params, state);
this.script = script;