Skip to content

Commit cbb799e

Browse files
rjernstrjernst
committed
Scripting: Add back params._source access in scripted metric aggs (elastic#34777) (elastic#34832)
Access to special variables _source and _fields were accidentally removed in recent refactorings. This commit adds them back, along with a test. closes elastic#33884
1 parent eb28e60 commit cbb799e

File tree

8 files changed

+174
-76
lines changed

8 files changed

+174
-76
lines changed

‎modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptedMetricAggContextsTests.java

+36-1
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,25 @@
2121

2222
import org.apache.lucene.search.DocIdSetIterator;
2323
import org.apache.lucene.search.Scorer;
24+
import org.apache.lucene.index.LeafReaderContext;
25+
import org.apache.lucene.index.memory.MemoryIndex;
2426
import org.elasticsearch.painless.spi.Whitelist;
2527
import org.elasticsearch.script.ScriptContext;
2628
import org.elasticsearch.script.ScriptedMetricAggContexts;
29+
import org.elasticsearch.search.lookup.LeafSearchLookup;
30+
import org.elasticsearch.search.lookup.SearchLookup;
31+
import org.elasticsearch.search.lookup.SourceLookup;
2732

33+
import java.io.IOException;
2834
import java.util.ArrayList;
2935
import java.util.Collections;
3036
import java.util.HashMap;
3137
import java.util.List;
3238
import java.util.Map;
3339

40+
import static org.mockito.Mockito.mock;
41+
import static org.mockito.Mockito.when;
42+
3443
public class ScriptedMetricAggContextsTests extends ScriptTestCase {
3544
@Override
3645
protected Map<ScriptContext<?>, List<Whitelist>> scriptContexts() {
@@ -58,7 +67,7 @@ public void testInitBasic() {
5867
assertEquals(10, state.get("testField"));
5968
}
6069

61-
public void testMapBasic() {
70+
public void testMapBasic() throws IOException {
6271
ScriptedMetricAggContexts.MapScript.Factory factory = scriptEngine.compile("test",
6372
"state.testField = 2*_score", ScriptedMetricAggContexts.MapScript.CONTEXT, Collections.emptyMap());
6473

@@ -86,6 +95,32 @@ public void testMapBasic() {
8695
assertEquals(1.0, state.get("testField"));
8796
}
8897

98+
public void testMapSourceAccess() throws IOException {
99+
ScriptedMetricAggContexts.MapScript.Factory factory = scriptEngine.compile("test",
100+
"state.testField = params._source.three", ScriptedMetricAggContexts.MapScript.CONTEXT, Collections.emptyMap());
101+
102+
Map<String, Object> params = new HashMap<>();
103+
Map<String, Object> state = new HashMap<>();
104+
105+
MemoryIndex index = new MemoryIndex();
106+
// we don't need a real index, just need to construct a LeafReaderContext which cannot be mocked
107+
LeafReaderContext leafReaderContext = index.createSearcher().getIndexReader().leaves().get(0);
108+
109+
SearchLookup lookup = mock(SearchLookup.class);
110+
LeafSearchLookup leafLookup = mock(LeafSearchLookup.class);
111+
when(lookup.getLeafSearchLookup(leafReaderContext)).thenReturn(leafLookup);
112+
SourceLookup sourceLookup = mock(SourceLookup.class);
113+
when(leafLookup.asMap()).thenReturn(Collections.singletonMap("_source", sourceLookup));
114+
when(sourceLookup.get("three")).thenReturn(3);
115+
ScriptedMetricAggContexts.MapScript.LeafFactory leafFactory = factory.newFactory(params, state, lookup);
116+
ScriptedMetricAggContexts.MapScript script = leafFactory.newInstance(leafReaderContext);
117+
118+
script.execute();
119+
120+
assert(state.containsKey("testField"));
121+
assertEquals(3, state.get("testField"));
122+
}
123+
89124
public void testCombineBasic() {
90125
ScriptedMetricAggContexts.CombineScript.Factory factory = scriptEngine.compile("test",
91126
"state.testField = params.initialVal; return state.testField + params.inc", ScriptedMetricAggContexts.CombineScript.CONTEXT,

‎server/src/main/java/org/elasticsearch/script/ScriptedMetricAggContexts.java

+91-33
Original file line numberDiff line numberDiff line change
@@ -22,42 +22,35 @@
2222
import org.apache.lucene.index.LeafReaderContext;
2323
import org.apache.lucene.search.Scorer;
2424
import org.elasticsearch.ElasticsearchException;
25-
import org.elasticsearch.common.logging.DeprecationLogger;
26-
import org.elasticsearch.common.logging.Loggers;
2725
import org.elasticsearch.index.fielddata.ScriptDocValues;
2826
import org.elasticsearch.search.lookup.LeafSearchLookup;
2927
import org.elasticsearch.search.lookup.SearchLookup;
3028

3129
import java.io.IOException;
30+
import java.util.Collections;
31+
import java.util.HashMap;
3232
import java.util.List;
3333
import java.util.Map;
3434

3535
public class ScriptedMetricAggContexts {
36-
private static final DeprecationLogger DEPRECATION_LOGGER =
37-
new DeprecationLogger(Loggers.getLogger(ScriptedMetricAggContexts.class));
3836

39-
// Public for access from tests
40-
public static final String AGG_PARAM_DEPRECATION_WARNING =
41-
"params._agg/_aggs for scripted metric aggregations are deprecated, use state/states (not in params) instead. " +
42-
"Use -Des.aggregations.enable_scripted_metric_agg_param=false to disable.";
43-
44-
public static boolean deprecatedAggParamEnabled() {
45-
boolean enabled = Boolean.parseBoolean(
46-
System.getProperty("es.aggregations.enable_scripted_metric_agg_param", "true"));
47-
48-
if (enabled) {
49-
DEPRECATION_LOGGER.deprecatedAndMaybeLog("enable_scripted_metric_agg_param", AGG_PARAM_DEPRECATION_WARNING);
37+
public abstract static class InitScript {
38+
private static final Map<String, String> DEPRECATIONS;
39+
static {
40+
Map<String, String> deprecations = new HashMap<>();
41+
deprecations.put(
42+
"_agg",
43+
"Accessing variable [_agg] via [params._agg] from within a scripted metric agg init script " +
44+
"is deprecated in favor of using [state]."
45+
);
46+
DEPRECATIONS = Collections.unmodifiableMap(deprecations);
5047
}
5148

52-
return enabled;
53-
}
54-
55-
private abstract static class ParamsAndStateBase {
5649
private final Map<String, Object> params;
5750
private final Object state;
5851

59-
ParamsAndStateBase(Map<String, Object> params, Object state) {
60-
this.params = params;
52+
public InitScript(Map<String, Object> params, Object state) {
53+
this.params = new ParameterMap(params, DEPRECATIONS);
6154
this.state = state;
6255
}
6356

@@ -68,12 +61,6 @@ public Map<String, Object> getParams() {
6861
public Object getState() {
6962
return state;
7063
}
71-
}
72-
73-
public abstract static class InitScript extends ParamsAndStateBase {
74-
public InitScript(Map<String, Object> params, Object state) {
75-
super(params, state);
76-
}
7764

7865
public abstract void execute();
7966

@@ -85,14 +72,51 @@ public interface Factory {
8572
public static ScriptContext<Factory> CONTEXT = new ScriptContext<>("aggs_init", Factory.class);
8673
}
8774

88-
public abstract static class MapScript extends ParamsAndStateBase {
75+
public abstract static class MapScript {
76+
private static final Map<String, String> DEPRECATIONS;
77+
78+
static {
79+
Map<String, String> deprecations = new HashMap<>();
80+
deprecations.put(
81+
"doc",
82+
"Accessing variable [doc] via [params.doc] from within a scripted metric agg map script " +
83+
"is deprecated in favor of directly accessing [doc]."
84+
);
85+
deprecations.put(
86+
"_doc",
87+
"Accessing variable [doc] via [params._doc] from within a scripted metric agg map script " +
88+
"is deprecated in favor of directly accessing [doc]."
89+
);
90+
deprecations.put(
91+
"_agg",
92+
"Accessing variable [_agg] via [params._agg] from within a scripted metric agg map script " +
93+
"is deprecated in favor of using [state]."
94+
);
95+
DEPRECATIONS = Collections.unmodifiableMap(deprecations);
96+
}
97+
98+
private final Map<String, Object> params;
99+
private final Object state;
89100
private final LeafSearchLookup leafLookup;
90101
private Scorer scorer;
91102

92103
public MapScript(Map<String, Object> params, Object state, SearchLookup lookup, LeafReaderContext leafContext) {
93-
super(params, state);
94-
104+
this.state = state;
95105
this.leafLookup = leafContext == null ? null : lookup.getLeafSearchLookup(leafContext);
106+
if (leafLookup != null) {
107+
params = new HashMap<>(params); // copy params so we aren't modifying input
108+
params.putAll(leafLookup.asMap()); // add lookup vars
109+
params = new ParameterMap(params, DEPRECATIONS); // wrap with deprecations
110+
}
111+
this.params = params;
112+
}
113+
114+
public Map<String, Object> getParams() {
115+
return params;
116+
}
117+
118+
public Object getState() {
119+
return state;
96120
}
97121

98122
// Return the doc as a map (instead of LeafDocLookup) in order to abide by type whitelisting rules for
@@ -138,9 +162,32 @@ public interface Factory {
138162
public static ScriptContext<Factory> CONTEXT = new ScriptContext<>("aggs_map", Factory.class);
139163
}
140164

141-
public abstract static class CombineScript extends ParamsAndStateBase {
165+
public abstract static class CombineScript {
166+
private static final Map<String, String> DEPRECATIONS;
167+
static {
168+
Map<String, String> deprecations = new HashMap<>();
169+
deprecations.put(
170+
"_agg",
171+
"Accessing variable [_agg] via [params._agg] from within a scripted metric agg combine script " +
172+
"is deprecated in favor of using [state]."
173+
);
174+
DEPRECATIONS = Collections.unmodifiableMap(deprecations);
175+
}
176+
177+
private final Map<String, Object> params;
178+
private final Object state;
179+
142180
public CombineScript(Map<String, Object> params, Object state) {
143-
super(params, state);
181+
this.params = new ParameterMap(params, DEPRECATIONS);
182+
this.state = state;
183+
}
184+
185+
public Map<String, Object> getParams() {
186+
return params;
187+
}
188+
189+
public Object getState() {
190+
return state;
144191
}
145192

146193
public abstract Object execute();
@@ -154,11 +201,22 @@ public interface Factory {
154201
}
155202

156203
public abstract static class ReduceScript {
204+
private static final Map<String, String> DEPRECATIONS;
205+
static {
206+
Map<String, String> deprecations = new HashMap<>();
207+
deprecations.put(
208+
"_aggs",
209+
"Accessing variable [_aggs] via [params._aggs] from within a scripted metric agg reduce script " +
210+
"is deprecated in favor of using [state]."
211+
);
212+
DEPRECATIONS = Collections.unmodifiableMap(deprecations);
213+
}
214+
157215
private final Map<String, Object> params;
158216
private final List<Object> states;
159217

160218
public ReduceScript(Map<String, Object> params, List<Object> states) {
161-
this.params = params;
219+
this.params = new ParameterMap(params, DEPRECATIONS);
162220
this.states = states;
163221
}
164222

‎server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/InternalScriptedMetric.java

+1-3
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,7 @@ public InternalAggregation doReduce(List<InternalAggregation> aggregations, Redu
9696
}
9797

9898
// Add _aggs to params map for backwards compatibility (redundant with a context variable on the ReduceScript created below).
99-
if (ScriptedMetricAggContexts.deprecatedAggParamEnabled()) {
100-
params.put("_aggs", aggregationObjects);
101-
}
99+
params.put("_aggs", aggregationObjects);
102100

103101
ScriptedMetricAggContexts.ReduceScript.Factory factory = reduceContext.scriptService().compile(
104102
firstAggregation.reduceScript, ScriptedMetricAggContexts.ReduceScript.CONTEXT);

‎server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java

+7-9
Original file line numberDiff line numberDiff line change
@@ -84,15 +84,13 @@ public Aggregator createInternal(Aggregator parent, boolean collectsFromSingleBu
8484
// When this is removed, aggState (as passed to ScriptedMetricAggregator) can be changed to Map<String, Object>, since
8585
// it won't be possible to completely replace it with another type as is possible when it's an entry in params.
8686
Object aggState = new HashMap<String, Object>();
87-
if (ScriptedMetricAggContexts.deprecatedAggParamEnabled()) {
88-
if (aggParams.containsKey("_agg") == false) {
89-
// Add _agg if it wasn't added manually
90-
aggParams.put("_agg", aggState);
91-
} else {
92-
// If it was added manually, also use it for the agg context variable to reduce the likelihood of
93-
// weird behavior due to multiple different variables.
94-
aggState = aggParams.get("_agg");
95-
}
87+
if (aggParams.containsKey("_agg") == false) {
88+
// Add _agg if it wasn't added manually
89+
aggParams.put("_agg", aggState);
90+
} else {
91+
// If it was added manually, also use it for the agg context variable to reduce the likelihood of
92+
// weird behavior due to multiple different variables.
93+
aggState = aggParams.get("_agg");
9694
}
9795

9896
final ScriptedMetricAggContexts.InitScript initScript = this.initScript.newInstance(

‎server/src/test/java/org/elasticsearch/search/aggregations/metrics/scripted/InternalScriptedMetricAggStateV6CompatTests.java

+5-5
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import org.elasticsearch.common.settings.Settings;
2424
import org.elasticsearch.script.MockScriptEngine;
2525
import org.elasticsearch.script.Script;
26-
import org.elasticsearch.script.ScriptedMetricAggContexts;
2726
import org.elasticsearch.script.ScriptEngine;
2827
import org.elasticsearch.script.ScriptModule;
2928
import org.elasticsearch.script.ScriptService;
@@ -63,9 +62,9 @@ protected InternalScriptedMetric createTestInstance(String name, List<PipelineAg
6362
*/
6463
@Override
6564
protected ScriptService mockScriptService() {
66-
Function<Map<String, Object>, Object> script = params -> {
67-
Object aggs = params.get("_aggs");
68-
Object states = params.get("states");
65+
Function<Map<String, Object>, Object> script = vars -> {
66+
Object aggs = ((Map<String,Object>) vars.get("params")).get("_aggs");
67+
Object states = vars.get("states");
6968
assertThat(aggs, instanceOf(List.class));
7069
assertThat(aggs, sameInstance(states));
7170
return aggs;
@@ -80,7 +79,8 @@ protected ScriptService mockScriptService() {
8079

8180
@Override
8281
protected void assertReduced(InternalScriptedMetric reduced, List<InternalScriptedMetric> inputs) {
83-
assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
82+
assertWarnings("Accessing variable [_aggs] via [params._aggs] from within a scripted metric agg reduce script " +
83+
"is deprecated in favor of using [state].");
8484
}
8585

8686
@Override

0 commit comments

Comments
 (0)