You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metron.apache.org by cestella <gi...@git.apache.org> on 2017/09/12 15:18:28 UTC
[GitHub] metron pull request #750: METRON-1179: Make STATS_ADD to take a list
GitHub user cestella opened a pull request:
https://github.com/apache/metron/pull/750
METRON-1179: Make STATS_ADD to take a list
## Contributor Comments
While we can add multiple values in `STATS_ADD` via `STATS_ADD(s, 1, 2, 3)`, but it'd be nice also to support `STATS_ADD(s, [1, 2, 3])`
## Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our [Development Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235) for the complete guide to follow for contributions.
Please refer also to our [Build Verification Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview) for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
- [ ] Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
### For code changes:
- [ ] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
- [ ] Have you included steps or a guide to how the change may be verified and tested manually?
- [ ] Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
```
mvn -q clean integration-test install && build_utils/verify_licenses.sh
```
- [ ] Have you written or updated unit tests and or integration tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via `site-book/target/site/index.html`:
```
cd site-book
mvn site
```
#### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that [travis-ci](https://travis-ci.org) is set up for your personal repository such that your branches are built there before submitting a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cestella/incubator-metron stats_add_list
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/750.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #750
----
commit 53ab5ef225c0ba74f7d58f43e3123a7cc325efe7
Author: cstella <ce...@gmail.com>
Date: 2017-09-12T15:04:45Z
Add STATS_ADD_ALL
commit 72b93b678d782c5f67eee2fbf75c3ebadf82196a
Author: cstella <ce...@gmail.com>
Date: 2017-09-12T15:16:24Z
last revision.
commit 55a616e79e1c9feeb894b0442f220192d5192d9e
Author: cstella <ce...@gmail.com>
Date: 2017-09-12T15:18:15Z
Updating
----
---
[GitHub] metron issue #750: METRON-1179: Make STATS_ADD to take a list
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/750
Looks great. Just a couple of nits, but otherwise LGTM +1.
---
[GitHub] metron pull request #750: METRON-1179: Make STATS_ADD to take a list
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/750#discussion_r138506017
--- Diff: metron-analytics/metron-statistics/src/main/java/org/apache/metron/statistics/StellarStatisticsFunctions.java ---
@@ -141,8 +141,21 @@ public Object apply(List<Object> args) {
// add each of the numeric values
for(int i=1; i<args.size(); i++) {
- double value = convert(args.get(i), Double.class);
- stats.addValue(value);
+ Object n = args.get(i);
+ if( n != null) {
+ if(n instanceof Iterable) {
+ for(Object num : (Iterable<Object>)n) {
+ if(num != null) {
--- End diff --
Just a nit, but why 'double' and not Double?
Is there a reason to box?
---
[GitHub] metron issue #750: METRON-1179: Make STATS_ADD to take a list
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/750
+1, thanks for the contribution and great work.
---
[GitHub] metron pull request #750: METRON-1179: Make STATS_ADD to take a list
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on a diff in the pull request:
https://github.com/apache/metron/pull/750#discussion_r138725654
--- Diff: metron-analytics/metron-statistics/src/main/java/org/apache/metron/statistics/StellarStatisticsFunctions.java ---
@@ -141,8 +141,21 @@ public Object apply(List<Object> args) {
// add each of the numeric values
for(int i=1; i<args.size(); i++) {
- double value = convert(args.get(i), Double.class);
- stats.addValue(value);
+ Object n = args.get(i);
+ if( n != null) {
+ if(n instanceof Iterable) {
+ for(Object num : (Iterable<Object>)n) {
+ if(num != null) {
--- End diff --
No good reason :)
---
[GitHub] metron pull request #750: METRON-1179: Make STATS_ADD to take a list
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metron/pull/750
---
[GitHub] metron pull request #750: METRON-1179: Make STATS_ADD to take a list
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/750#discussion_r138506239
--- Diff: metron-analytics/metron-statistics/README.md ---
@@ -60,7 +60,7 @@ functions can be used from everywhere where Stellar is used.
* Description: Adds one or more input values to those that are used to calculate the summary statistics.
* Input:
* stats - The Stellar statistics object. If null, then a new one is initialized.
- * value+ - One or more numbers to add
+ * value+ - One or more numbers to add. If one of objects is a list, it will iterate over the list and add each number.
--- End diff --
"One or more items to add. Each item may be a number or a list of numbers. If an item is a list, each number in the list will be added"?
---
[GitHub] metron issue #750: METRON-1179: Make STATS_ADD to take a list
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/metron/pull/750
Ok, this is updated as per your comments, @ottobackwards
---