Enhancements to map_to_list processor #4033
Conversation
Signed-off-by: Hai Yan <[email protected]>
Signed-off-by: Hai Yan <[email protected]>
Signed-off-by: Hai Yan <[email protected]>
oeyh
requested review from
chenqi0805,
engechas,
graytaylor0,
dinujoh,
kkondaka,
asifsmohammed and
dlvenable
as code owners
| final Map<String, Object> sourceMap; | ||
| if (config.getSource() == null) { | ||
| // Source is root | ||
| sourceMap = OBJECT_MAPPER.treeToValue(recordEvent.getJsonNode(), Map.class); |
There was a problem hiding this comment.
I have a high level question here, if you get the entire JSON node if source is null, I think it's not going to work. Correct me if I am wrong.
For example, if the following is the event.
{
"my-map": {
"key1": "value1",
"key2": "value2",
"key3": "value3"
}
}
The result will be
[
["my-map", {
"key1": "value1",
"key2": "value2",
"key3": "value3"
}]
]
There was a problem hiding this comment.
Good point. I think adding test cases and documentation for euch cases would help.
There was a problem hiding this comment.
@asifsmohammed Good question, though it isn't necessarily related to root being the source. I think it's an issue with nested map in the source map. Currently we assume the source map structure is flat, so what you show is the expected result. We can have a recursive option to go deeper into each field in the future if needed.
There was a problem hiding this comment.
Added readme and more test cases.
| final Map<String, Object> sourceMap; | ||
| if (config.getSource() == null) { | ||
| // Source is root | ||
| sourceMap = OBJECT_MAPPER.treeToValue(recordEvent.getJsonNode(), Map.class); |
There was a problem hiding this comment.
Good point. I think adding test cases and documentation for euch cases would help.
| } | ||
| } | ||
| return records; | ||
| } | ||
|
|
||
| private Map<String, Object> getSourceMap(Event recordEvent) throws JsonProcessingException { | ||
| final Map<String, Object> sourceMap; | ||
| if (config.getSource() == null) { |
There was a problem hiding this comment.
Is this a desirable feature?
If so, can we make it take in / instead of null? I think this could lead to confusion when users don't set the source field they are probably not getting what they want.
There was a problem hiding this comment.
Yes, we will need it for #3965.
If so, can we make it take in / instead of null? I think this could lead to confusion when users don't set the source field they are probably not getting what they want.
In JsonPointer specification, it is actually the empty string "" that refers to the whole document and "/" refers to the field with "" as key. If we remove the restrictions in JacksonEvent that key cannot be empty string, we should already support referring to the root document with Json Pointer "". What do you think of doing it this way?
There was a problem hiding this comment.
Did some testing if we use "/" as source, it will try to get the value with empty string as key.
There was a problem hiding this comment.
@oeyh , That is a very useful finding.
I think we will need to make the syntax be explicit then:
source: ''
There was a problem hiding this comment.
Implemented this in recent commits.
Signed-off-by: Hai Yan <[email protected]>
Signed-off-by: Hai Yan <[email protected]>
Signed-off-by: Hai Yan <[email protected]>
Signed-off-by: Hai Yan <[email protected]>
Signed-off-by: Hai Yan <[email protected]>
Signed-off-by: Hai Yan <[email protected]>
| } else { | ||
| jsonPointerKey = SEPARATOR + key; | ||
| } | ||
| return jsonNode.at(jsonPointerKey); |
There was a problem hiding this comment.
Is this optimization we are doing? If yes, why not modify toJsonPointer() function? Or delete that function if that's not needed anymoer?
There was a problem hiding this comment.
Good point. Moved the changes to toJsonPointer() method. In the process, found some corner case issues with previous changes, addressed those and added more unit tests.
…ore tests Signed-off-by: Hai Yan <[email protected]>
oeyh
force-pushed
the
conv-object-to-list-in-mtl
branch
from
d3a05dd to
7e7b3a3
Compare
Signed-off-by: Hai Yan <[email protected]>
Description
Adds some enhancements to to map_to_list processor:
convert_field_to_listoption. If true, will convert fields to lists ([key, value]) instead of objects ({key: value})sourceis not specifiedtags_on_failureoption to add tags when events failed to processIssues Resolved
Contributes to #3965
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.