Scripted_metric _agg parameter disappears if params are provided #19863
Conversation
…ams are provided
clintongormley
changed the title
|
@colings86 could you take a look please? |
|
@consulthys Thanks for the PR. Would you be able to add a test to ScriptedMetricIT to test your change? |
|
@colings86 Yes, definitely, I will do this and report when done. |
|
Can one of the admins verify this patch? |
|
I've (finally) committed a test case for this PR. Sorry it took so long. |
|
@elasticmachine test this please |
| @@ -67,6 +67,8 @@ public Aggregator createInternal(Aggregator parent, boolean collectsFromSingleBu | |||
| params = deepCopyParams(params, context); | |||
| } else { | |||
| params = new HashMap<>(); | |||
| } | |||
| if (!params.containsKey("_agg")) { | |||
There was a problem hiding this comment.
one minor thing, we try to be consistent and don't use ! to negate a boolean. Yet we try to use == false since it's easier to read ! is easy to miss. Would you make that change?
There was a problem hiding this comment.
Oh I missed that, of course, I'll do it
|
@consulthys thanks for fixing the comment so quickly. I will wait for CI and pull it in afterwards. |
|
@elasticmachine test this please |
|
@simonw for some reason I pushed the wrong version of the test case, sorry about that. |
|
@elasticmachine ok to test |
|
@elasticmachine test this please |
|
@consulthys can you take another look, you test failed here https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request+multijob-intake/2847/console |
|
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? |
1 similar comment
|
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? |
|
@consulthys Are you still interested in getting this PR merged? The test you added fails on your final assert of the method, on the total count equaling the number of docs. |
|
Yes, I'm still interested. I got sidetracked, I will put this back onto the stack. |
|
@consulthys ping again on this, are you still interested in this PR? |
|
@dakrone Yes, I am, sorry for the delay |
|
This would be very helpful, any update on this? |
|
@consulthys I'm going to close this PR for now as it appears stalled. Please reopen when/if you have time to bring it up to date and fix the test. |
|
@rjernst @consulthys would you be opposed if I went ahead and fixed this so it can (hopefully) get merged in? |
|
@reeselevine please do open a new PR! |
|
@reeselevine Please feel free to do this in a new PR. |
Quick fix to
scripted_metricaggregation to provide a more intuitive behavior whenparamswithout_aggare being specified.Closes #19768