Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scripted metric aggs ignore script level params #28819

Closed
rjernst opened this issue Feb 25, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@rjernst
Copy link
Member

commented Feb 25, 2018

When scripts are specified, either in json or via new Script(), they take a params Map. Scripted metrics use multiple scripts, and take a params map at the outer level which applies to all of the 4 scripts of a scripted metric agg. However, the params passed directly to a script are lost.

I traced this to a flaw in ScriptedMetricAggregationBuilder. The original Script object for each script, eg mapScript, needs to be passed through (or at least the params from it) to the ScriptedMetricAggregatorFactory, and then before running each script, the relevant per script params need to be merged in (by creating a new map and added the per script params).

@rjernst

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2018

/cc @elastic/es-search-aggs

@rationull

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2018

@rjernst What should the behavior be if script params and aggregation params are provided with the same name?

I'm inclined to think that should cause an error at ScriptedMetricAggregatorFactory construction time. If conflicts must be silently allowed then I think the aggregation level params have to win (at least for _agg to ensure it keeps the same identity across all the scripts that are run -- the general case is to ensure that all aggregation level params have the same identity across scripts, which is directly in conflict with letting individual scripts override such params). That works too but could be confusing and a fail-fast check ensuring no conflicts seems more developer friendly.

I have a start to a PR on this. Pretty sure I have it working but it needs some trivial cleanup plus refinement for this conflict case. Input is appreciated if you'd welcome a PR.

@rjernst

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2018

I think an error is the right thing. Right now, anything passed into params at the script level is ignored, so any errors that result of this change could only happen if a user is already either passing in params in both the script and one level up, or they are passing in _agg/_aggs at the script level, and they really should be notified that this is completely broken from what they are trying to do (and they can stop doing this before upgrading).

Long term, I think _agg and _aggs need to be removed from params, and replaced with specific ScriptContexts for each of the 4 script types for a metric agg. This would allow making the variables local to each script (no more need to access via params) and at the same time possibly renaming to be more descriptive. But a PR to fix this existing bug in the current setup would be greatly appreciated.

@rationull

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2018

Thanks, that makes sense. Agree that the aggregation status/results are really part of the context, not part of the parameters -- that does sound like a better design.

ScriptedMetricIT has a few test cases where the same params are being passed in at both the aggregator and script level (looks like by accident) so we'll get easy test coverage of the new error case :)

I will clean up my progress other than this new error case today and should be able to take care of the error case either this week or next weekend and submit the PR.

@rationull

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2018

(Another option is we could treat _agg/_aggs as a special case to disallow passing at the script level, but given your long term change suggestion that is not really moving in the right direction in general. Even passing non-"special" params in at both the aggregate and script level seems more like a confusing mistake than an important use case.)

@rjernst

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2018

/cc @colings86 for your thoughts on the approach discussed here.

rationull added a commit to rationull/elasticsearch that referenced this issue Mar 19, 2018

Pass script level params into scripted metric aggs (elastic#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.

rationull added a commit to rationull/elasticsearch that referenced this issue Mar 19, 2018

Disallow duplicate parameter names between scripted agg and script (e…
…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.
@colings86

This comment has been minimized.

Copy link
Member

commented Mar 19, 2018

Agreed that an error would be the best approach for handling colliding keys in the params. I also agree that now we have script contexts it would be much better to expose the _agg and _aggs variables there rather than abusing the params for this.

colings86 added a commit that referenced this issue Apr 3, 2018

Pass through script params in scripted metric agg (#29154)
* 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 added a commit that referenced this issue Apr 4, 2018

Pass through script params in scripted metric agg (#29154)
* 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.
@rationull

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

@rjernst can this be closed?

@rjernst

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2018

Yes, closed by #29154.

@rjernst rjernst closed this Jun 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.