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

Bug fix to allow access to top level params in reduce script #42096

Merged
merged 8 commits into from
May 23, 2019

Conversation

jdconrad
Copy link
Contributor

Parameters specified for use in all scripts within a metric aggregation were never merged into reduce scripts as described in this issue (#42046). This fix merges top-level parameters for a metric aggregation into reduce script parameters.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo

@jdconrad jdconrad requested a review from rjernst May 20, 2019 15:01
@jdconrad
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/2

@jdconrad
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/bwc

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -72,6 +72,8 @@
Collections.singletonMap("itemValue", 12));
private static final Script COMBINE_SCRIPT_PARAMS = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "combineScriptParams",
Collections.singletonMap("divisor", 4));
private static final Script REDUCE_SCRIPT_AGG_PARAMS = new Script(ScriptType.INLINE, MockScriptEngine.NAME, "reduceScriptAggParams",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be named without "agg" like the similar constants here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed.

@jdconrad
Copy link
Contributor Author

@rjernst Thanks for the review! Will commit as soon as CI passes.

@jdconrad
Copy link
Contributor Author

@elasticmachine test this please

@jdconrad
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

@jdconrad
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants