Improved aggrMethod to accept multiple values #605
Conversation
|
Hi @fgalan , If PR seems ok, please merge the PR. |
CHANGES_NEXT_RELEASE
Outdated
| @@ -1 +1,2 @@ | |||
| - Improved aggrMethod to accept multiple values (#432, partial) | |||
There was a problem hiding this comment.
| - Improved aggrMethod to accept multiple values (#432, partial) | |
| - Add: aggrMethod accept multiple values separated by comma (#432, partial) | |
| - Add: aggrMethod 'all' to get all the possible aggregations (#432, partial) |
In addition, documentation at doc/manuals/aggregated-data-retrieval.md (aggrMethod bullet) should be modified to explain the new functionality.
There was a problem hiding this comment.
Resolved in ee9cded
| @@ -912,21 +912,22 @@ function aggregatedDataAvailableSinceDateTest(ngsiVersion, params, done) { | |||
| break; | |||
| } | |||
|
|
|||
| let value; | |||
There was a problem hiding this comment.
Instead of modifying existing tests, please add new ones. In particular, I'd suggest two new tests cases:
- Using the 'all' token
- Using two aggregation methods (whatever) separated by ","
There was a problem hiding this comment.
Hi @fgalan,
I have added the new test cases here.
Previously, only one aggrMethod was there so switch case was working fine. But in case of multiple aggregation method we need to update the code for multiple values separately. The above code change is for util class to accommodate the multiple aggregation methods.
|
Hi @fgalan , Thanks |
lib/server/sthServer.js
Outdated
| const list = ['min', 'max', 'sum', 'sum2', 'occur', 'all']; | ||
| const joinedList = '(' + list.join('|') + ')'; | ||
| const regex = new RegExp('^' + joinedList + '(,' + joinedList + ')*$'); |
There was a problem hiding this comment.
Please use more meaningful names names for the variables. For instance:
| const list = ['min', 'max', 'sum', 'sum2', 'occur', 'all']; | |
| const joinedList = '(' + list.join('|') + ')'; | |
| const regex = new RegExp('^' + joinedList + '(,' + joinedList + ')*$'); | |
| const aggList = ['min', 'max', 'sum', 'sum2', 'occur', 'all']; | |
| const joinedAggList = '(' + aggList.join('|') + ')'; | |
| const aggRegex = new RegExp('^' + joinedAggList + '(,' + joinedAggList + ')*$'); |
| provided by these aggregated methods with the number of samples, it is possible to calculate probabilistic values | ||
| such as the average value, the variance as well as the standard deviation. It is a mandatory parameter. | ||
| samples) for numeric attribute values and `occur` for attributes values of type string. It accepts multiple values | ||
| separated by comma (min,max) to get multiple aggregation method values. Additionally, `aggrMethod=all` can be used |
There was a problem hiding this comment.
| separated by comma (min,max) to get multiple aggregation method values. Additionally, `aggrMethod=all` can be used | |
| separated by comma (eg. `aggrMethod=min,max`) to get multiple aggregation method values. Additionally, `aggrMethod=all` can be used |
| @@ -324,13 +324,45 @@ describe('sth tests', function() { | |||
| }) | |||
| ); | |||
|
|
|||
| it( | |||
There was a problem hiding this comment.
I'd suggest to add a couple of cases:
- should respond with 400 - Bad Request if aggMethod is not a valid one (e.g. aggMethod=foo)
- should respond with 400 - Bad Request if aggMethod mixes a valid one with a not valid one (eg. aggMethod=max,foo)
Btw, what happens if aggMethod mixes all with another method? Eg. aggMethod=all,min? Do we get and error or the "min" part is ignored and that's equivalent to aggMethod=all?
There was a problem hiding this comment.
Added test cases in 3b51b4c
If aggMethod mixes all with another method e.g. aggMethod=all,min, then it is equivalent to aggMethod=all ignoring all other parameters.
|
Hi @fgalan , I have updated all the suggestions mentioned by you in previous comment. |
Thanks for your contribution! We would have a look to the PR the soon as possible. |
|
Hi @fgalan , Could you please have a look on this PR? |
|
Is this a new feature or extends and modify a current one? |
Hi @AlvaroVega , Documentation for this is already updated in this PR. |
Fixes #432 partially.
aggrMethod can take multiple aggregations using comma separation.