Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add ARRAY_CONCAT UDF #7761

Merged
merged 1 commit into from
Jul 12, 2021
Merged

feat: add ARRAY_CONCAT UDF #7761

merged 1 commit into from
Jul 12, 2021

Conversation

patrickstuedi
Copy link
Contributor

Description

Adding array_concat UDF function.

DESCRIBE function array_concat:

Name        : ARRAY_CONCAT
Overview    : Concatenates two arrays, creating an array that contains all the elementsin the first array followed by all the elements in the second array.
Type        : SCALAR
Jar         : internal
Variations  : 

        Variation   : ARRAY_CONCAT(left ARRAY<T>, right ARRAY<T>)
        Returns     : ARRAY<T>
        left        : First array of values
        right       : Second array of values

Testing done

Tested with different array values, including duplicates, null value array members and null value input parameters

@patrickstuedi patrickstuedi requested review from JimGalasyn and a team as code owners July 5, 2021 14:17
@ghost
Copy link

ghost commented Jul 5, 2021

@confluentinc It looks like @patrickstuedi just signed our Contributor License Agreement. 👍

Always at your service,

clabot

JimGalasyn
JimGalasyn approved these changes Jul 6, 2021
View reviewed changes
Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

vvcephei
vvcephei reviewed Jul 6, 2021
View reviewed changes
Copy link
Member

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Just a couple of thoughts.

ksqldb-engine/src/test/java/io/confluent/ksql/function/udf/array/ArrayConcatTest.java Outdated Show resolved Hide resolved
ksqldb-engine/src/main/java/io/confluent/ksql/function/udf/array/ArrayConcat.java Outdated Show resolved Hide resolved
@patrickstuedi patrickstuedi force-pushed the array_concat2 branch 4 times, most recently from 8b10c90 to e421153 Compare July 7, 2021 13:07
agavra
agavra reviewed Jul 7, 2021
View reviewed changes
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

took a quick scan of the PR - thanks @patrickstuedi!

ksqldb-engine/src/main/java/io/confluent/ksql/function/udf/array/ArrayConcat.java Outdated
if (left == null && right == null) {
return null;
}
final List<T> result = new ArrayList(left.size() + right.size());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if left == null || right == null wouldn't we get an NPE here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought so too, but there are unit tests for those cases. Not sure what to make of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for pointing out. missed that when changing the initialization to the exact number of elements. No idea what's going on with the tests, they should fail here. Will double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests did catch this, sorry for missing to run them on that last change. Update fixes the issue in ArrayConcat.

ksqldb-functional-tests/src/test/resources/query-validation-tests/array-set-functions.json
"CREATE STREAM OUTPUT AS SELECT id, array_concat(arr1, arr2) as result FROM INPUT;"
],
"inputs": [
{"topic": "test_topic", "key": "r1", "value": {"arr1": [0,0,1,0,-1], "arr2": [1,-2,0]}},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's also add the null cases here (left is null, right is null, both are null)

vvcephei
vvcephei reviewed Jul 7, 2021
View reviewed changes
docs/developer-guide/ksqldb-reference/scalar-functions.md Outdated

Returns an array representing the concatenation of both input arrays.

Returns NULL if both input arrays are NULL.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Returns NULL if both input arrays are NULL.
Returns NULL if both input arrays are NULL. If only one argument is NULL, the result is the other argument.

... Or any other way you prefer to say it, but we should specify the behavior if just one arg is null.

@patrickstuedi patrickstuedi force-pushed the array_concat2 branch 6 times, most recently from 6151326 to ca419e4 Compare July 9, 2021 08:45
@vvcephei
Copy link
Member

vvcephei commented Jul 9, 2021

I just checked on the test failures: https://jenkins.confluent.io/job/Confluentinc%20Contributors/job/ksql/job/PR-7761/7/#showFailuresLink

It looks like the same set of tests started failing on master yesterday: https://jenkins.confluent.io/job/confluentinc/job/ksql/job/master/7992/#showFailuresLink

vvcephei
vvcephei approved these changes Jul 9, 2021
View reviewed changes
Copy link
Member

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @patrickstuedi !

@patrickstuedi patrickstuedi merged commit 1de9ef8 into confluentinc:master Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agavra agavra agavra left review comments

@vvcephei vvcephei vvcephei approved these changes

@JimGalasyn JimGalasyn JimGalasyn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@patrickstuedi @vvcephei @agavra @JimGalasyn