Improving parsing of sigma param for Extended Stats Bucket Aggregation #17562
Conversation
clintongormley
added
>bug
review
:Analytics/Aggregations
| } | ||
| } else { | ||
| leftover.put(currentFieldName, parser.objectText()); | ||
| parseToken(pipelineAggregatorName, parser, context, currentFieldName, token, params); |
There was a problem hiding this comment.
This reason for using the leftover map here is so we can provide all the incorrect fields in one error message rather than reporting one error at a time and making the user potentially have to iterate multiple times to fix all their errors. Would you be able to revert these changes here so we again have these errors all reported in one go?
In the future I would like to extend this to the entire request and all parsing errors so we report all problems with the request at once.
There was a problem hiding this comment.
Actually looking more at the changes you have made I see that you have made the code more like the other aggregation parsers so I think I would like to keep these changes and implement the above in another way in the future.
|
@alexshadow007 Thanks for raising this PR. I left a couple of comments but I think this looks pretty good so far. |
|
@colings86 could review this again please? |
|
@alexshadow007 Sorry its taken a while for me to get back to you on this. It looks good to me but I wonder if you could update the branch with the latest master so it can be merged? |
|
@colings86 I updated branch with latest master. |
|
@alexshadow007 Thanks. I'm just going to run through a build again to check there is nothing unexpected and then I'll merge it in |
|
@alexshadow007 merged into master. Thanks for the contribution. Sorry it took so long to get reviewed and merged |
Also improving parsing of percents param for Percentiles Bucket Aggregation
Fix for #17499