You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by pvillard31 <gi...@git.apache.org> on 2017/09/01 13:16:44 UTC
[GitHub] nifi pull request #2122: NIFI-4340 - fix record path evaluation when array i...
GitHub user pvillard31 opened a pull request:
https://github.com/apache/nifi/pull/2122
NIFI-4340 - fix record path evaluation when array is [ null ]
Thank you for submitting a contribution to Apache NiFi.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with NIFI-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)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
- [ ] Have you written or updated unit 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)?
- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
- [ ] If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### 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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pvillard31/nifi NIFI-4340
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/2122.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 #2122
----
commit ad6423ee08720accf86acb14f059f9119a1d1ec8
Author: Pierre Villard <pi...@gmail.com>
Date: 2017-09-01T13:16:11Z
NIFI-4340 - fix record path evaluation when array is [ null ]
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #2122: NIFI-4340 - fix record path evaluation when array is [ nul...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/2122
Pushed a new commit, rebased and squashed to only address the initial NPE issue. Will address the array update as part of NIFI-4383.
---
[GitHub] nifi pull request #2122: NIFI-4340 - fix record path evaluation when array i...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/2122
---
[GitHub] nifi pull request #2122: NIFI-4340 - fix record path evaluation when array i...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2122#discussion_r137869296
--- Diff: nifi-commons/nifi-record-path/src/main/java/org/apache/nifi/record/path/paths/WildcardIndexPath.java ---
@@ -65,6 +65,7 @@
} else {
final Object[] array = (Object[]) value;
return IntStream.range(0, array.length)
+ .filter(index -> array[index] != null)
--- End diff --
@pvillard31 I don't think this is exactly right. The wildcard should include all elements, including nulls. Imagine, for example, that I have some data like:
```
{
"id" : 3,
"numbers" : [ 1, null, 4 ]
}
```
If I want to set /numbers[*] to the value 8, then I would expect to get back:
```
{
"id" : 3,
"numbers" : [ 8, 8, 8 ]
}
```
But with this modification I'd get back
```
{
"id" : 3,
"numbers" : [ 8, null, 8 ]
}
```
---
[GitHub] nifi pull request #2122: NIFI-4340 - fix record path evaluation when array i...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2122#discussion_r138177766
--- Diff: nifi-commons/nifi-record-path/src/main/java/org/apache/nifi/record/path/paths/WildcardIndexPath.java ---
@@ -65,9 +65,13 @@
} else {
final Object[] array = (Object[]) value;
return IntStream.range(0, array.length)
+ .filter(index -> array[index] != null
+ || (((ArrayDataType) fieldValue.getField().getDataType()).getElementType()).getFieldType() != RecordFieldType.RECORD)
--- End diff --
I pushed another commit to address it in ChildPath. Let me know if it sounds better.
---
[GitHub] nifi pull request #2122: NIFI-4340 - fix record path evaluation when array i...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2122#discussion_r138635543
--- Diff: nifi-commons/nifi-record-path/src/main/java/org/apache/nifi/record/path/paths/WildcardIndexPath.java ---
@@ -67,7 +67,9 @@
return IntStream.range(0, array.length)
.mapToObj(index -> {
final DataType elementDataType = ((ArrayDataType) fieldValue.getField().getDataType()).getElementType();
- final RecordField elementField = new RecordField(fieldValue.getField().getFieldName() + "[" + index + "]", elementDataType);
+ final RecordField elementField = elementDataType.getFieldType() == RecordFieldType.RECORD
--- End diff --
@pvillard31 can you help me to understand why we're doing this? If it is a record then we include the index in the name but otherwise we don't -- why not always include the index in the name?
---
[GitHub] nifi pull request #2122: NIFI-4340 - fix record path evaluation when array i...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2122#discussion_r138649045
--- Diff: nifi-commons/nifi-record-path/src/main/java/org/apache/nifi/record/path/paths/WildcardIndexPath.java ---
@@ -67,7 +67,9 @@
return IntStream.range(0, array.length)
.mapToObj(index -> {
final DataType elementDataType = ((ArrayDataType) fieldValue.getField().getDataType()).getElementType();
- final RecordField elementField = new RecordField(fieldValue.getField().getFieldName() + "[" + index + "]", elementDataType);
+ final RecordField elementField = elementDataType.getFieldType() == RecordFieldType.RECORD
--- End diff --
Ah ok. Yes, I think it should probably be a new JIRA. Because I suspect we will also have the same issue with ArrayIndexPath and MultiArrayIndexPath and possibly others...
---
[GitHub] nifi pull request #2122: NIFI-4340 - fix record path evaluation when array i...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2122#discussion_r137950192
--- Diff: nifi-commons/nifi-record-path/src/main/java/org/apache/nifi/record/path/paths/WildcardIndexPath.java ---
@@ -65,6 +65,7 @@
} else {
final Object[] array = (Object[]) value;
return IntStream.range(0, array.length)
+ .filter(index -> array[index] != null)
--- End diff --
Hey @markap14, thanks for the review. While I agree with your comment, it turns out (if I looked correctly) that, currently, updating an array of simple elements is not possible. If I set:
```
/numbers[*]
````
as the path to update with a value of 8, it won't update anything.
I rebased against master and pushed another commit to fix the update action when updating an array of simple (not records) elements, and also to address your comment. Let me know if it looks like the right approach to you.
---
[GitHub] nifi pull request #2122: NIFI-4340 - fix record path evaluation when array i...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2122#discussion_r138090059
--- Diff: nifi-commons/nifi-record-path/src/main/java/org/apache/nifi/record/path/paths/WildcardIndexPath.java ---
@@ -65,9 +65,13 @@
} else {
final Object[] array = (Object[]) value;
return IntStream.range(0, array.length)
+ .filter(index -> array[index] != null
+ || (((ArrayDataType) fieldValue.getField().getDataType()).getElementType()).getFieldType() != RecordFieldType.RECORD)
--- End diff --
@pvillard31 I think this still is not exactly what we need. Even a null record should be returned when given a Wildcard. I contend that in all cases we should return the array value, null or otherwise. I think perhaps the issue that you're hitting is not related to WildcardIndexPath but rather ChildPath / DescendantPath.
---
[GitHub] nifi pull request #2122: NIFI-4340 - fix record path evaluation when array i...
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/2122#discussion_r138642556
--- Diff: nifi-commons/nifi-record-path/src/main/java/org/apache/nifi/record/path/paths/WildcardIndexPath.java ---
@@ -67,7 +67,9 @@
return IntStream.range(0, array.length)
.mapToObj(index -> {
final DataType elementDataType = ((ArrayDataType) fieldValue.getField().getDataType()).getElementType();
- final RecordField elementField = new RecordField(fieldValue.getField().getFieldName() + "[" + index + "]", elementDataType);
+ final RecordField elementField = elementDataType.getFieldType() == RecordFieldType.RECORD
--- End diff --
This is not related to the NPE issue. It's related to my earlier comment about updating arrays of simple fields. Right now, if you try to update an array that does not contain records, it won't do anything because of the field name we're giving here. If you think that should be a separated JIRA and/or addressed differently, let me know. (I added a unit test to update an array of integers)
---
[GitHub] nifi issue #2122: NIFI-4340 - fix record path evaluation when array is [ nul...
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on the issue:
https://github.com/apache/nifi/pull/2122
All looks good. Thanks @pvillard31. Merged to master.
---