Pass through script params in scripted metric agg #29154

rationull · 2018-03-20T05:50:05Z

This PR attempts to fix issue #28819 by merging agg params and script params when building ScriptedMetricAggregator. Conflicting param names throw IllegalArgumentException.

Now params that are passed at the script level and at the aggregation level are merged and can both be used in the aggregation scripts. If there are any conflicts, aggregation level params will win. This may be followed by another change detecting that case and throwing an exception to disallow such conflicts.

…lastic#28819) If a scripted metric aggregation has aggregation params and script params which have the same name, throw an IllegalArgumentException when merging the parameter lists.

elasticmachine · 2018-03-20T05:50:07Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elasticmachine · 2018-03-20T05:50:07Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

rationull · 2018-03-20T05:51:28Z

server/src/test/java/org/elasticsearch/search/aggregations/metrics/ScriptedMetricIT.java

@@ -322,11 +325,11 @@ public void testMap() {
        assertThat(numShardsRun, greaterThan(0));
    }

-    public void testMapWithParams() {
+    public void testExplicitAggParam() {


testMapWithParams was previously actually working because the params were on the aggregation, not because the params were on the map script. This test may be extraneous now but I renamed it instead of deleting it since it does provide specific coverage for an explicit _agg param, even if that may be kind of a weird case. I wouldn't presume to delete it without an opinion from a dev more familiar with this code..

rationull · 2018-03-20T05:55:42Z

.../org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java

-        final ExecutableScript initScript = this.initScript.newInstance(params);
-        final SearchScript.LeafFactory mapScript = this.mapScript.newFactory(params, lookup);
-        final ExecutableScript combineScript = this.combineScript.newInstance(params);
+        final ExecutableScript initScript = this.initScript.newInstance(mergeParams(aggParams, initScriptParams));


I considered merging the parameter lists at construction time (which would also move conflict detection to construction time) but hit some test failures in that case. I did not rigorously track down the problem because I wasn't sure that it was a good idea to evaluate the params list before the last minute anyway, in case they could be modified in any way between AggregatorFactory construction and calling createInternal(). This is a smaller behavior change.

rationull · 2018-03-20T05:57:54Z

.../org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java


-    public ScriptedMetricAggregatorFactory(String name, SearchScript.Factory mapScript, ExecutableScript.Factory initScript,
-                                           ExecutableScript.Factory combineScript, Script reduceScript, Map<String, Object> params,
+    public ScriptedMetricAggregatorFactory(String name, SearchScript.Factory mapScript, Map<String, Object> mapScriptParams,


Would it make sense to combine together the compiled script factories and the params into some kind of a tuple object rather than just adding more constructor parameters? I opted not to because they're different types and AFAIK passing them around paired with parameters isn't a common case.

elasticmachine · 2018-03-20T10:19:39Z

Pinging @elastic/es-search-aggs

colings86 · 2018-03-26T09:04:54Z

jenkins please test this

rationull · 2018-03-28T05:23:08Z

@colings86 looking at the failure in Jenkins, I note that the previous build failed in what looks like the same way -- I don't see that it's actually related to these changes.

I just merged master into this feature branch with no conflicts, but I haven't had a chance to run any tests on it locally other than a quick precommit in a docker container (looks like I need to update to JDK10 first and I'm not at a good position to do that quite right now).

You want to kick it to jenkins again or wait until I get a chance to run more checks on it locally first?

colings86 · 2018-03-28T08:06:30Z

@rationull you are right, that failure doesn't look to be related to your changes, thanks for merging master I'll kick off the build again and we'll see what happens. If this same problem reoccurs I'll dig into whats going on but I suspect the problem has been resolved already.

colings86 · 2018-03-28T08:06:40Z

jenkins please retest this

colings86

@rationull thanks for working on this PR. It LGTM, I'm going to run the tests one more time and then I'll get this merged in

colings86 · 2018-03-29T07:08:04Z

jenkins retest this please

colings86 · 2018-04-03T08:59:37Z

@rationull sorry for the delay merging this, I have been out since Thursday. Thanks for making this fix, I've merged it to master and will backport to 6.x when I've confirmed the master branch is still green

* master: Reindex: Fix error in delete-by-query rest spec (elastic#29318) Improve similarity integration. (elastic#29187) Fix some query extraction bugs. (elastic#29283) [Docs] Correct experimental note formatting Move Nullable into core (elastic#29341) [Docs] Update getting-started.asciidoc (elastic#29294) Elasticsearch 6.3.0 is now on Lucene 7.3. [DOCS] Refer back to index API for full-document updates in _update API section (elastic#28677) Fix missing comma in ingest-node.asciidoc (elastic#29343) Improve exception handling on TransportMasterNodeAction (elastic#29314) Don't break allocation if resize source index is missing (elastic#29311) Use fixture to test repository-s3 plugin (elastic#29296) Fix NDCG for empty search results (elastic#29267) Pass through script params in scripted metric agg (elastic#29154) Fix Eclipse build. Upgrade to lucene-7.3.0-snapshot-98a6b3d. (elastic#29298) Painless: Remove extraneous INLINE constant. (elastic#29340)

* master: Build: Fix Java9 MR build (elastic#29312) Reindex: Fix error in delete-by-query rest spec (elastic#29318) Improve similarity integration. (elastic#29187) Fix some query extraction bugs. (elastic#29283) [Docs] Correct experimental note formatting Move Nullable into core (elastic#29341) [Docs] Update getting-started.asciidoc (elastic#29294) Elasticsearch 6.3.0 is now on Lucene 7.3. [DOCS] Refer back to index API for full-document updates in _update API section (elastic#28677) Fix missing comma in ingest-node.asciidoc (elastic#29343) Improve exception handling on TransportMasterNodeAction (elastic#29314) Don't break allocation if resize source index is missing (elastic#29311) Use fixture to test repository-s3 plugin (elastic#29296) Fix NDCG for empty search results (elastic#29267) Pass through script params in scripted metric agg (elastic#29154) Fix Eclipse build. Upgrade to lucene-7.3.0-snapshot-98a6b3d. (elastic#29298) Painless: Remove extraneous INLINE constant. (elastic#29340) Remove HTTP max content length leniency (elastic#29337) Begin moving XContent to a separate lib/artifact (elastic#29300)

* Pass script level params into scripted metric aggs (#28819) Now params that are passed at the script level and at the aggregation level are merged and can both be used in the aggregation scripts. If there are any conflicts, aggregation level params will win. This may be followed by another change detecting that case and throwing an exception to disallow such conflicts. * Disallow duplicate parameter names between scripted agg and script (#28819) If a scripted metric aggregation has aggregation params and script params which have the same name, throw an IllegalArgumentException when merging the parameter lists.

colings86 · 2018-04-04T09:23:25Z

backported to 6.x branch

* es/6.x: (68 commits) Add note to migration docs on silent batch mode (#29365) Allow using distance measure in the geo context precision (#29273) Disable failing query in QueryBuilderBWCIT. Improve similarity integration. (#29187) Fix some query extraction bugs. (#29283) Fixed quote_field_suffix in query_string (#29332) TEST: Update negative byte size setting error msg Fix bwc in GeoDistanceQuery serialization (#29325) Move testMappingConflictRootCause to different class Enhance error for out of bounds byte size settings (#29338) [Docs] Correct javadoc of GetIndexRequest (#29364) Check presence of multi-types before validating new mapping (#29316) Make TransportRankEvalAction members final Pass through script params in scripted metric agg (#29154) Remove silent batch mode from install plugin (#29359) Track Lucene operations in engine explicitly (#29357) Build: Fix Java9 MR build (#29312) Reindex: Fix error in delete-by-query rest spec (#29318) Move Nullable into core (#29341) [Docs] Correct experimental note formatting ...

rationull added 2 commits March 18, 2018 21:37

Disallow duplicate parameter names between scripted agg and script (e…

a41bc47

…lastic#28819) If a scripted metric aggregation has aggregation params and script params which have the same name, throw an IllegalArgumentException when merging the parameter lists.

rationull commented Mar 20, 2018

View reviewed changes

DaveCTurner changed the title ~~Fix #28819: Pass through script params in scripted metric agg~~ Pass through script params in scripted metric agg Mar 20, 2018

DaveCTurner added the :Analytics/Aggregations Aggregations label Mar 20, 2018

Merge branch 'master' into rationull-28819-merge-agg-and-script-params

d56fcf7

colings86 approved these changes Mar 29, 2018

View reviewed changes

colings86 added v7.0.0 v6.3.0 >bug labels Mar 29, 2018

colings86 merged commit 0028563 into elastic:master Apr 3, 2018

colings86 added the backport pending label Apr 4, 2018

colings86 removed the backport pending label Apr 4, 2018

rationull deleted the rationull-28819-merge-agg-and-script-params branch June 27, 2018 05:10

rjernst mentioned this pull request Jun 29, 2018

Scripted metric aggs ignore script level params #28819

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass through script params in scripted metric agg #29154

Pass through script params in scripted metric agg #29154

rationull commented Mar 20, 2018

elasticmachine commented Mar 20, 2018

elasticmachine commented Mar 20, 2018

rationull Mar 20, 2018 •

edited

Loading

rationull Mar 20, 2018

rationull Mar 20, 2018

elasticmachine commented Mar 20, 2018

colings86 commented Mar 26, 2018

rationull commented Mar 28, 2018

colings86 commented Mar 28, 2018

colings86 commented Mar 28, 2018

colings86 left a comment

colings86 commented Mar 29, 2018

colings86 commented Apr 3, 2018

colings86 commented Apr 4, 2018

Pass through script params in scripted metric agg #29154

Pass through script params in scripted metric agg #29154

Conversation

rationull commented Mar 20, 2018

elasticmachine commented Mar 20, 2018

elasticmachine commented Mar 20, 2018

rationull Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

rationull Mar 20, 2018

Choose a reason for hiding this comment

rationull Mar 20, 2018

Choose a reason for hiding this comment

elasticmachine commented Mar 20, 2018

colings86 commented Mar 26, 2018

rationull commented Mar 28, 2018

colings86 commented Mar 28, 2018

colings86 commented Mar 28, 2018

colings86 left a comment

Choose a reason for hiding this comment

colings86 commented Mar 29, 2018

colings86 commented Apr 3, 2018

colings86 commented Apr 4, 2018

rationull Mar 20, 2018 •

edited

Loading