[UDF] JSON Array Contains function #608
Conversation
|
@satybald - there is an internal nexus issue - it will be resolved in the next couple of hours |
| KsqlFunction jsonArrayContainsString = new KsqlFunction( | ||
| Schema.BOOLEAN_SCHEMA, Arrays.asList(array, Schema.STRING_SCHEMA), | ||
| "JSON_ARRAY_CONTAINS", JsonArrayContainsKudf.class); | ||
| addFunction(jsonArrayContainsString); |
There was a problem hiding this comment.
I have an issue registering my function passing arguments to it. I'm expecting that 'evaluate' method passes json string as first argment of the array, and the second argument is search value.
|
thanks @bluemonk3y for information. Is there any way of getting the result of Jenkins build? sorry @satybald - ATM its behind a confluent firewall. We are looking at other ways of notifying build failures |
|
@satybald I would suggest to change this function to search general array type regardless if the serialization mode is JSON or AVRO. This way this can be used in broader use cases. |
|
@hjafarpour thanks for the suggestion. I added support for the general array type. |
| jsonUdf.init(); | ||
| } | ||
|
|
||
| public void testEmptyArray() |
| } | ||
|
|
||
| public void testIfJsonArrayContainsLongValue() | ||
| { |
There was a problem hiding this comment.
As above, missing Test annotation.
| } | ||
|
|
||
| public void testIfArrayContainsDoubleValue() | ||
| { |
There was a problem hiding this comment.
As above, missing Test annotation.
| assertEquals(false, jsonUdf.evaluate("[[2.0], 3.0]", 2.0)); | ||
| } | ||
|
|
||
| public void testIfArrayContainsStringValue() |
There was a problem hiding this comment.
As above, missing Test annotation.
The same for all tests methods.
|
@hjafarpour thanks for the feedback. Added @test annotation. |
satybald
changed the title
| @@ -151,6 +153,27 @@ private void init() { | |||
| "EXTRACTJSONFIELD", JsonExtractStringKudf.class); | |||
| addFunction(getStringFromJson); | |||
|
|
|||
| KsqlFunction jsonArrayContainsString = new KsqlFunction( | |||
| Schema.BOOLEAN_SCHEMA, Arrays.asList(Schema.STRING_SCHEMA, Schema.STRING_SCHEMA), | |||
| "JSON_ARRAY_CONTAINS", JsonArrayContainsKudf.class); | |||
There was a problem hiding this comment.
I have doubts should name contains underscore or not? For consistency with EXTRACTJSONFIELD it should be without, but for readability, I argue that underscore is needed.
There was a problem hiding this comment.
@satybald - it needs to be consistent for this PR. EXTRACTJSONFIELD - changing formatting will require doc changes etc - which could be done in another PR
There was a problem hiding this comment.
@bluemonk3y good point about the doc. I completely forgot to document function 😄
|
@hjafarpour @dguy what do you think about PR? I compile and test branch locally build passes all the tests. |
| } | ||
| Object searchValue = args[1]; | ||
| if(args[0] instanceof String) { | ||
| String jsonString = args[0].toString(); |
There was a problem hiding this comment.
perhaps just cast it to string and inline in ifJsonStringArrayContains
| throw new KsqlFunctionException("Invalid type parameters for " + Arrays.toString(args)); | ||
| } | ||
|
|
||
| private Object ifArrayContains(Object[] array, Object searchValue) { |
There was a problem hiding this comment.
return boolean and rename to arrayContains
|
|
||
| private Object ifArrayContains(Object[] array, Object searchValue) { | ||
| //TODO: Refactor this to ArrayUtil.containsValue from TopK PR#575 | ||
| for (Object value : array) { |
There was a problem hiding this comment.
return Arrays.stream(array).anyMatch(value -> Objects.equals(o, searchValue))
| return false; | ||
| } | ||
|
|
||
| private boolean ifJsonStringArrayContains(String json, Object searchValue) { |
There was a problem hiding this comment.
rename to stringArrayContains
| while (parser.currentToken() != null) { | ||
| JsonToken token = parser.nextToken(); | ||
| if (token == null) { | ||
| return token == searchValue; |
| @Test | ||
| public void testEmptyArray() | ||
| { | ||
| assertEquals(false, jsonUdf.evaluate("[]", true)); |
| } | ||
|
|
||
| @Test | ||
| public void testNullArray() |
There was a problem hiding this comment.
split this into two tests, i.e., one for when it should be true and on for when it should be false. Also naming as proposed above
| @Test | ||
| public void testNullArray() | ||
| { | ||
| assertEquals(true, jsonUdf.evaluate("[null]", null)); |
| } | ||
|
|
||
| @Test | ||
| public void testIntegerJsonArrayContainsValues() |
| } | ||
|
|
||
| @Test | ||
| public void testIfJsonArrayContainsLongValue() |
|
@dguy thanks for the review. I've applied your feedback. However, I cannot use assertTrue() and assertFalse() as jsonUdf.evaluate returns Object. I need explicitly state against which object I'm evaluating. |
|
@satybald with your latest updates it looks like you have bought in a bunch of changes that aren't yours, i.e, Dockerfile, pom.xml etc |
|
@dguy I did rebase with a master to add |
|
@satybald - those changes shouldn't show up in this diff, though. They should already be in master |
satybald
force-pushed
the
feature-array-contains
branch
3 times, most recently
from
dbc6b82 to
866fdce
Compare
satybald
force-pushed
the
feature-array-contains
branch
from
866fdce to
ca6af94
Compare
|
retest this please |
|
@dguy I fixed that now and cherry-picked need me commit with |
|
@satybald ..engine/src/main/java/io/confluent/ksql/function/udf/json/ArrayContainsKudf.java:29:8: Unused import - java.util.Objects. [UnusedImports] |
satybald
force-pushed
the
feature-array-contains
branch
from
542154e to
c0208ad
Compare
|
thanks, @bluemonk3y. fixed the issue. |
|
@dguy @bluemonk3y @hjafarpour build again failed 😢 |
|
Thanks @satybald, the build has passed so i've merged this PR. Thanks for the contribution |
UDF function that searches if the value exists in JSON Array. Function accepts two arguments a JSON string, and a searchValue. SearchValue can be any of the BOOLEAN, INTEGER, BIGINT, DOUBLE and VARCHAR. Fixes #607