Modify Key Value processor to support string literal grouping #4599
Conversation
Signed-off-by: Krishna Kondaka <[email protected]>
Signed-off-by: Krishna Kondaka <[email protected]>
Signed-off-by: Krishna Kondaka <[email protected]>
kkondaka
requested review from
chenqi0805,
engechas,
graytaylor0,
dinujoh,
asifsmohammed,
KarstenSchnitter,
dlvenable and
oeyh
as code owners
Signed-off-by: Krishna Kondaka <[email protected]>
Signed-off-by: Krishna Kondaka <[email protected]>
Signed-off-by: Krishna Kondaka <[email protected]>
| this.stringLiteralCharacter = keyValueProcessorConfig.getStringLiteralCharacter(); | ||
| if (stringLiteralCharacter != null) { | ||
| if (!keyValueProcessorConfig.getValueGrouping()) { | ||
| LOG.warn("string literal character config is ignored becuase value grouping is disabled"); |
There was a problem hiding this comment.
Should we fail on this instead? We could add an @AssertTrue statement in the config class.
| LOG.warn("string literal character config is ignored becuase value grouping is disabled"); | ||
| } | ||
| if (stringLiteralCharacter != '\"' && stringLiteralCharacter != '\'') { | ||
| throw new RuntimeException("Only single quote and double quote are supported as string literal character"); |
There was a problem hiding this comment.
| throw new RuntimeException("Only single quote and double quote are supported as string literal character"); | |
| throw new RuntimeException("Only single quote (') or double quote (") are supported as string literal character"); |
There was a problem hiding this comment.
Also, you should move this validation into the config class. This will help with our goals for more dynamic pipeline configurations.
| @JsonProperty("strict_grouping") | ||
| private boolean strictGrouping = false; | ||
|
|
||
| @JsonProperty("string_literal_character") |
There was a problem hiding this comment.
I think we should name this string_literal_characters and make it a String. But only allow a certain sieze.
| @JsonProperty("string_literal_character") | |
| @Size(min = 0, max = 1, message = "field_split_characters may only have character") | |
| @JsonProperty("string_literal_characters") |
| @@ -340,7 +367,7 @@ public Collection<Record<Event>> doExecute(final Collection<Record<Event>> recor | |||
| continue; | |||
| } | |||
|
|
|||
| final String groupsRaw = recordEvent.get(keyValueProcessorConfig.getSource(), String.class); | |||
| String groupsRaw = recordEvent.get(keyValueProcessorConfig.getSource(), String.class); | |||
There was a problem hiding this comment.
It does not look like you modify this. You can keep the final modifier.
| @@ -489,10 +516,19 @@ private Map<String, Object> createNonRecursedMap(String[] groups) { | |||
| List<Object> valueList; | |||
|
|
|||
| for(final String group : groups) { | |||
| // If a group starts and ends with stringLiteralCharacter, | |||
| // treat the entire group as key with null as the value | |||
| if (stringLiteralCharacter != null && group.charAt(0) == stringLiteralCharacter && group.charAt(group.length()-1) == stringLiteralCharacter) { | |||
There was a problem hiding this comment.
Maybe refactor this into a method with a clear name to explain what it is:
private boolean isIgnoredGroup(String group) {
return stringLiteralCharacter != null && group.charAt(0) == stringLiteralCharacter && group.charAt(group.length()-1) == stringLiteralCharacter;
}
| @@ -187,6 +188,7 @@ void testDropKeysWithNoValue() { | |||
| @MethodSource("getKeyValueGroupingTestdata") | |||
| void testMultipleKvToObjectKeyValueProcessorWithValueGrouping(String fieldDelimiters, String input, Map<String, Object> expectedResultMap) { | |||
| lenient().when(mockConfig.getValueGrouping()).thenReturn(true); | |||
| lenient().when(mockConfig.getStringLiteralCharacter()).thenReturn('\"'); | |||
There was a problem hiding this comment.
I tend to think we should test these both with and without the string literal character.
You could include that as an argument in the test dynamically. Or split the test into two.
There was a problem hiding this comment.
I have modified the Test testStringLiteralCharacter to test with both characters instead of this test.
Signed-off-by: Krishna Kondaka <[email protected]>
| @@ -261,11 +264,14 @@ public int skipGroup(final String str, int idx, final Character endChar) { | |||
| i++; | |||
| continue; | |||
| } else if (str.charAt(i) == endChar) { | |||
| return i-1; | |||
There was a problem hiding this comment.
Looks like a previous PR did this but this method doesn't need to be public
Description
Adds the following new features
drop_keys_with_no_valuesuch text can be dropped from the outputstrict_groupingoption where a group with the starting group character and missing end group character does not throw error and instead considered as the group till the end of the textvalue_groupingto strictly apply grouping to values and not to keys and other parts of input.Issues Resolved
Resolves #[Issue number to be closed when this PR is merged]
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.