Add array_has_duplicates Presto function #3320
Conversation
✅ Deploy Preview for meta-velox ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
facebook-github-bot
added
the
CLA Signed
There was a problem hiding this comment.
@duanmeng Any particular reason to implement this function as a vector function? It seems it can be implemented as a simple function, no?
duanmeng
force-pushed
the
array_has_duplicates
branch
from
fca6f7d
to
9d29e6e
Compare
|
|
@duanmeng I suggest to implement this function as a simple function. (ArrayDuplicates and ArrayContains may have been implemented before simple functions supported complex types. It would be nice to convert these to simple functions as well.) |
@mbasmanova Got it, I will change my implementation. |
duanmeng
force-pushed
the
array_has_duplicates
branch
from
9d29e6e
to
2f91505
Compare
|
@mbasmanova I've updated the implementation using simple functions, could you please help to review? |
There was a problem hiding this comment.
@duanmeng Thank you for iterating on this PR. Overall looks good.
Please, update the PR title to use the template from https://github.com/facebookincubator/velox/blob/main/CONTRIBUTING.md#prestos-sql-functions and add documentation.
Please, also run Fuzzer:
velox_expression_fuzzer_test --logtostderr=1 --velox_fuzzer_enable_complex_types --only array_has_duplicates --duration_sec 60
velox_expression_fuzzer_test --logtostderr=1 --velox_fuzzer_enable_complex_types --enable_variadic_signatures --duration_sec 600
velox/functions/prestosql/registration/ArrayFunctionsRegistration.cpp
Outdated
Show resolved
Hide resolved
|
|
||
| auto array = makeNullableArrayVector<StringView>({ | ||
| {}, | ||
| {S("")}, |
duanmeng
changed the title
duanmeng
force-pushed
the
array_has_duplicates
branch
2 times, most recently
from
2c0867a
to
cfeaf07
Compare
|
@mbasmanova I've resolved the comments and updated the PR title, and doc, and ran the two fuzzer tests following your guidance. The partial log of the two tests Succeed |
|
@mbasmanova Could you please help to take a look? |
|
@xiaoxmeng @Yuhta Could you help to review this pr, I've resolved all the comments from @mbasmanova, she seems to be on vacation. |
duanmeng
force-pushed
the
array_has_duplicates
branch
from
cfeaf07
to
73667f0
Compare
There was a problem hiding this comment.
@duanmeng Looks good to me. Thank you for the contribution.
|
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mbasmanova Thanks for your help on my first add function pr in velox:). Can I do something about the Facebook internal builds & tests? |
| }); | ||
| auto expected = makeFlatVector<bool>({false, true, false, true}); | ||
| testExpr(expected, "array_has_duplicates(C0)", {array}); | ||
| } |
There was a problem hiding this comment.
Thanks for the contribution, @duanmeng. I have closed my duplicate PR. Feel free to borrow more unit test cases, such as constant vectors, nonContiguousRows, here.
There was a problem hiding this comment.
Thanks for the contribution, @duanmeng. I have closed my duplicate PR. Feel free to borrow more unit test cases, such as constant vectors, nonContiguousRows, here.
@darrenfu Thanks for your suggestion, I used to add the constant and non Contiguous Rows test case referring to the test case in Array Duplicates Test and removed them according to the comments "This test is redundant as this logic is processed by the framework".
|
@duanmeng Thank you for the contribution. |
No description provided.