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

Add contains_sequence function #5593

Closed
wants to merge 1 commit into from
Closed

Add contains_sequence function #5593

wants to merge 1 commit into from

Conversation

tinkerrrr
Copy link
Contributor

@tinkerrrr tinkerrrr commented Oct 19, 2020 •
edited
Loading

Add a function determines whether an array contains another, with the values in the exact order.

select contains_subarray(array['1','2','3','4'], array['2',3'])  -> true
select contains_subarray(array['1','2','3','4'], array['1',5'])  -> false
select contains_subarray(array['1','2','3','4'], array['1',3'])  -> false

@cla-bot cla-bot bot added the cla-signed label Oct 19, 2020
@findepi findepi requested a review from dain October 20, 2020 08:13
dain
dain requested changes Oct 20, 2020
View reviewed changes
Copy link
Member

@dain dain left a comment

Choose a reason for hiding this comment

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

If this is just contains all, I'm not sure we need a new function, as match_all covers this. If the goal is contains sub-sequence, then the algorithm will need changes.

presto-main/src/main/java/io/prestosql/operator/scalar/ArrayContainsSubarray.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/ArrayContainsSubarray.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/ArrayContainsSubarray.java Outdated Show resolved Hide resolved
presto-main/src/test/java/io/prestosql/operator/scalar/TestArrayContainsSubarray.java Outdated Show resolved Hide resolved
@martint
Copy link
Member

martint commented Oct 21, 2020 •
edited
Loading

I find the name contains_subarray somewhat misleading, as it seems to convey that it's searching for arrays within an array of arrays. It would be more precise to name it contains_sequence, IMO.

@tinkerrrr
Copy link
Contributor Author

I find the name contains_subarray somewhat misleading, as it seems to convey that it's searching for arrays within an array of arrays. It would be more precise to name it contains_sequence, IMO.

Seems reasonable, how do you think? @dain

@tinkerrrr tinkerrrr requested a review from dain October 22, 2020 06:59
@dain
Copy link
Member

dain commented Oct 22, 2020 •
edited
Loading

Oh ya, I forgot to mention, we'll need documentation in this file: https://github.com/prestosql/presto/blob/master/presto-docs/src/main/sphinx/functions/array.rst

dain
dain requested changes Oct 22, 2020
View reviewed changes
Copy link
Member

@dain dain left a comment

Choose a reason for hiding this comment

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

Couple of minor things, but otherwise looks good. We'll still want to change to contains_sequence, and the documentation. Also, please squash this to a single commit, and update the commit message to reflect the new function name.

presto-main/src/main/java/io/prestosql/operator/scalar/ArrayContainsSubarray.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/ArrayContainsSubarray.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/operator/scalar/ArrayContainsSubarray.java Outdated Show resolved Hide resolved
@tinkerrrr tinkerrrr changed the title Add contains_subarray function Add contains_sequence function Oct 23, 2020
@tinkerrrr tinkerrrr requested a review from dain October 23, 2020 08:40
@tinkerrrr tinkerrrr force-pushed the contains_subarray branch 2 times, most recently from a9b02a6 to b28e814 Compare November 2, 2020 01:47
@tinkerrrr
Copy link
Contributor Author

ping @dain

@martint
Copy link
Member

martint commented Nov 9, 2020

Syntax looks good to me

dain
dain requested changes Nov 9, 2020
View reviewed changes
Copy link
Member

@dain dain left a comment

Choose a reason for hiding this comment

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

Two minor changes (one from me and one from @electrum) and this is ready to go.

presto-main/src/test/java/io/prestosql/operator/scalar/TestArrayContainsSequence.java
public void testBasic()
{
assertFunction("contains_sequence(ARRAY [1,2,3,4,5,6], ARRAY[1,2])", BOOLEAN, true);
assertFunction("contains_sequence(ARRAY [1,2,3,4,5,6], ARRAY[3,4])", BOOLEAN, true);
Copy link
Member

Choose a reason for hiding this comment

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

Can you add assertFunction("contains_sequence(ARRAY [1,2,3,4,5,6], ARRAY[5,6])", BOOLEAN, true); and

for (int i =1; i <=6; i++) {
    assertFunction("contains_sequence(ARRAY [1,2,3,4,5,6], ARRAY[" + i + "])", BOOLEAN, true);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@tinkerrrr tinkerrrr requested a review from dain November 11, 2020 07:42
@dain
Copy link
Member

dain commented Nov 12, 2020

@tinkerrrr can you squash your commits... you ended up with an extra commit named "Merge remote-tracking branch 'upstream/master' into contains_subarray"

@tinkerrrr
Add contains_sequence function
88a12bc 
fix if array contains NULL

Make the function truly contains_subarray

change contains_subarray to contains_sequence

Add UT
@tinkerrrr
Copy link
Contributor Author

@tinkerrrr can you squash your commits... you ended up with an extra commit named "Merge remote-tracking branch 'upstream/master' into contains_subarray"

@dain Done, I did that to try to resolve CI failure...

@dain
Copy link
Member

dain commented Nov 12, 2020

Merged, thanks!

@dain dain closed this Nov 12, 2020
@dain dain mentioned this pull request Nov 12, 2020
Closed
10 tasks
@martint martint added this to the 347 milestone Nov 16, 2020
@AngersZhuuuu AngersZhuuuu mentioned this pull request Nov 18, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@electrum electrum electrum left review comments

@findepi findepi findepi left review comments

@dain dain dain approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
347
Development

Successfully merging this pull request may close these issues.
5 participants
@tinkerrrr @martint @dain @electrum @findepi