Add shuffle Presto function #3404

darrenfu · 2022-12-02T19:12:25Z

shuffle(x) -> array

Generate a random permutation of the given array x.

For example:

SELECT shuffle(cast(array[1,2,2,3,4,null,5, null] as array(int))) -- [null, 1, 3, 4, 2, 5, 2, null] or any other permutation
SELECT shuffle(cast(array[null, null] as array(int))) -- [null, null]
SELECT shuffle(array['a', 'a', 'a']) -- ['a', 'a', 'a']
SELECT shuffle(cast(null as array(int))) -- null

NOTE: this is a non-deterministic function, hence, it may return different results for the same input even when input is a constant or dictionary encoded vector.

netlify · 2022-12-02T19:12:30Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`5b75e78`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/63cee0bafed3b8000893dea9

velox/vector/tests/utils/VectorTestBase.cpp

mbasmanova

@darrenfu Thank you for working on adding shuffle function. On top of regular usage, it could help enhance testing for array_sort function by allowing to generate random input then evaluate array_sort(x) and array_sort(shuffle(x)) and assert the results are the same.

CC: @kagamiori

velox/docs/functions/coverage.rst

velox/functions/prestosql/registration/ArrayFunctionsRegistration.cpp

velox/functions/prestosql/ArrayFunctions.h

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

darrenfu

Rewrite unit tests to better test the randomness and correctness of shuffle.

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

velox/functions/prestosql/ArrayFunctions.h

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

velox/functions/prestosql/ArrayFunctions.h

mbasmanova · 2022-12-12T16:12:58Z

@darrenfu I'm concerned about not adding support for all types and not implementing this as zero-copy for strings and complex types. What's preventing us from implementing this properly from the start, perhaps, using Vector function?

darrenfu · 2022-12-12T17:44:26Z

@darrenfu Thank you for working on adding shuffle function. On top of regular usage, it could help enhance testing for array_sort function by allowing to generate random input then evaluate array_sort(x) and array_sort(shuffle(x)) and assert the results are the same.

Good callout, @mbasmanova! Apart from the randomness test you mentioned, here I run shuffle(x) 100 times with a ranged-based input {0, 1, ..., 9}, and assert:

randomness: if the output shuffle arrays will be different at least 70% times in total. (actually, in this case, it should be > 99%)
correctness: if it's a permutation of the origin array. I'm using std::is_permutation() whose time complexity is O(N^2), a bit slower. Since it's unit test, I think it'll be fine.

This borrows some ideas from Presto java version: https://github.com/prestodb/presto/blob/master/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java#L4322. Let me know if the new tests suffices.

darrenfu · 2022-12-12T19:23:49Z

@darrenfu I'm concerned about not adding support for all types and not implementing this as zero-copy for strings and complex types. What's preventing us from implementing this properly from the start, perhaps, using Vector function?

I chatted with @laithsakka, looks like generic types are not supported yet in simple function api, I will re-implement it via vector function api. Thanks!

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

darrenfu · 2023-01-11T17:49:33Z

@darrenfu Darren, thank you for iterating on this PR. shuffle turns out to be a non-trivial function to implement. The code looks good to me, but the test seems overly complicated. I didn't have time to read it closely, but given that the function implementation doesn't depend on the type of the array elements, it seems redundant to test different element types.

@mbasmanova - I've removed all type related unit test cases. Let me know if you have any other feedback about the tests.

mbasmanova · 2023-01-11T17:53:22Z

@darrenfu There are CI failures. Would you take a look?

darrenfu · 2023-01-11T23:19:29Z

@darrenfu There are CI failures. Would you take a look?

I rebased from main branch to include #3633 to skip non-deterministic udf verification on the fuzzer side. And the ci failure has gone away.

@mbasmanova would you give a final stamp on this PR?

mbasmanova

@darrenfu The implementation of the shuffle function looks great. I have some comments about the test.

velox/functions/prestosql/ArrayShuffle.cpp

mbasmanova · 2023-01-18T14:48:25Z

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

+namespace {
+DecodedVector* decode(DecodedVector& decoder, const BaseVector& vector) {
+  SelectivityVector rows(vector.size());
+  decoder.decode(vector, rows);


Use decoder.decode(vector) to decode all rows. If you do that, then you won't need this helper function as it becomes a one-liner.

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

mbasmanova · 2023-01-18T15:10:24Z

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

+      DecodedVector decoded;
+      exec::VectorReader<Any> reader(decode(decoded, *arrayVectorResult));
+      auto arrayReader = reader[0].castTo<Array<int32_t>>();
+      std::vector<int32_t> shuffledResult(kNums);


Any particular reason not to use VectorReader::materialize?

Thanks for raising it.
One gotcha: since my input and output arrays are both null-free, I found instead of calling reader[0], I need to call reader.readNullFree(0) to force it return a std::vector<int32_t>.

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

mbasmanova · 2023-01-18T15:12:53Z

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

+             std::nullopt,
+             std::nullopt,
+         }});
+    std::vector<std::vector<std::optional<int64_t>>> expected{


Why can't we use auto expected = actual"?

Also, can we modify testIsPermutation to testShuffle(input) and have it take a single ArrayVector as input, evaluate shuffle on it and verify that result is a permutation of input?

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

darrenfu · 2023-01-19T08:08:11Z

@mbasmanova thanks for your scrutiny on the unit test class. I addressed all your comments, and besides the shuffle correctness (is-a-permutation), I also added three more test cases to cover the shuffle randomness with different encoding inputs. Please see the details in: testShuffleRandomness()

mbasmanova

@darrenfu Some additional comments on the test. I'm seeing a number of method with more parameters then needed / used.

mbasmanova · 2023-01-19T14:30:09Z

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

+  constexpr static const int32_t kNums = 10;
+  constexpr static const double kUniquenessRate = .7;
+
+  template <typename T, bool returnsOptionalValues = false>


All the calls to testShuffle specify returnsOptionalValues as true. Hence, this template parameter is not needed. Let's remove it.

Makes sense.
FYI: my original idea is to have only one testShuffle() function instead of two functions: testShuffle() + testShuffleRandomness(). Somehow, I need to deal with build errors caused by 1) having std::optional and nested std::vector in a folly::F14FastSet and 2) calculating the right sizes & thresholds for different shape of input array cases. Then, I ended up choosing to separate into two functions to keep brevity.

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

mbasmanova · 2023-01-19T14:32:31Z

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

+
+    for (auto i = 0; i < input->size(); i++) {
+      // Must materialize into std::vector, otherwise it will throw error
+      // because ArrayView doesn't support std::is_permutation() yet.


@laithsakka Would it make sense to add support for std::is_permutation() and similar to Array reader?

I agree. I updated the origin enhancement task with this requirement:
#3426

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

mbasmanova · 2023-01-19T14:42:00Z

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

+}
+
+TEST_F(ArrayShuffleTest, dictEncoding) {
+  // Considering the case of repeated elements: {1,2,3} x 3, {4,5} x 2.


This use case doesn't appear to be tested. Would you double check?

What do you mean "doesn't appear to be tested"?
Right now, I'm using std::is_permutation() to test if the shuffled output is a permutation of the origin input in test_shuffle() for the correctness purposes.
Regarding the randomness test, you will need to look at the test case: dictEncodingRandomness. It will generate a dictionary with dictionaryValues (size=200) and indices (size=100). The dictionaryValues' elements will all be identical: {0,1,2,...}. And it will test if the shuffled output contains at least X (= n * 70%) distinct permutations (e.g. {1,2,3},{1,2,3},... => {1,3,2}, {3,2,1}).
Putting in another word, if we shuffle only once for those repeated dict elements, it will yield one permutation (e.g. {1,2,3} => {1,3,2}), which will be captured by this assertion above. Does this new test case cover your question?

** UPDATE **
Will add duplicate elements in the dict indices to cover that.

velox/functions/prestosql/tests/ArrayShuffleTest.cpp

darrenfu · 2023-01-20T19:36:15Z

With the latest test cases: constantEncoding and constantEncodingRandomness, if revert to special handling on constant encoding (cc0cbd7), it will report the following error. It means these two test cases are necessary, and we shouldn't do any special handling on constant encoding as in other array UDFs.

unknown file: Failure
C++ exception with description "Exception: VeloxRuntimeError
Error Source: RUNTIME
Error Code: INVALID_STATE
Reason: Expression evaluation result is not of expected type: shuffle(C0) -> CONSTANT vector of type ARRAY<BIGINT>
Retriable: False
Expression: castedResult
Function: castEvaluateResult
File: /Users/dofu/git/velox/./velox/functions/prestosql/tests/utils/FunctionBaseTest.h
Line: 274

Thanks @mbasmanova to call it out.

mbasmanova

@darrenfu Thank you, Darren. Consider updating PR description to clarify that shuffle is a non-deterministic function, hence, it may return different results for the same input even when input is a constant or dictionary encoded vector.

facebook-github-bot · 2023-01-20T20:32:17Z

@darrenfu has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: `shuffle(x) -> array` > Generate a random permutation of the given array x. For example: ``` SELECT shuffle(cast(array[1,2,2,3,4,null,5, null] as array(int))) -- [null, 1, 3, 4, 2, 5, 2, null] or any other permutation SELECT shuffle(cast(array[null, null] as array(int))) -- [null, null] SELECT shuffle(array['a', 'a', 'a']) -- ['a', 'a', 'a'] SELECT shuffle(cast(null as array(int))) -- null ``` NOTE: this is a non-deterministic function, hence, it may return different results for the same input even when input is a constant or dictionary encoded vector. Pull Request resolved: facebookincubator#3404 Differential Revision: D42646294 Pulled By: darrenfu fbshipit-source-id: ca4052758ae485c5a4522298d2a35d445f2857d7

facebook-github-bot · 2023-01-23T18:51:58Z

This pull request was exported from Phabricator. Differential Revision: D42646294

Summary: `shuffle(x) -> array` > Generate a random permutation of the given array x. For example: ``` SELECT shuffle(cast(array[1,2,2,3,4,null,5, null] as array(int))) -- [null, 1, 3, 4, 2, 5, 2, null] or any other permutation SELECT shuffle(cast(array[null, null] as array(int))) -- [null, null] SELECT shuffle(array['a', 'a', 'a']) -- ['a', 'a', 'a'] SELECT shuffle(cast(null as array(int))) -- null ``` NOTE: this is a non-deterministic function, hence, it may return different results for the same input even when input is a constant or dictionary encoded vector. Pull Request resolved: facebookincubator#3404 Differential Revision: D42646294 Pulled By: darrenfu fbshipit-source-id: 04b23d67e0d82d708f12470b552c4772b1ecfc04

facebook-github-bot · 2023-01-23T19:32:08Z

This pull request was exported from Phabricator. Differential Revision: D42646294

facebook-github-bot · 2023-01-25T15:58:08Z

@darrenfu merged this pull request in 64f090d.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 2, 2022

darrenfu marked this pull request as draft December 2, 2022 19:12

darrenfu mentioned this pull request Dec 2, 2022

Add Presto functions #2262

Open

darrenfu force-pushed the array_shuffle_simpleudf branch 2 times, most recently from 0c576a9 to b6277e4 Compare December 2, 2022 20:01

darrenfu marked this pull request as ready for review December 2, 2022 20:02

darrenfu requested review from mbasmanova and kgpai December 2, 2022 20:04

darrenfu force-pushed the array_shuffle_simpleudf branch from b6277e4 to 95e3238 Compare December 2, 2022 20:12

darrenfu commented Dec 2, 2022

View reviewed changes

velox/vector/tests/utils/VectorTestBase.cpp Outdated Show resolved Hide resolved

mbasmanova reviewed Dec 5, 2022

View reviewed changes

darrenfu force-pushed the array_shuffle_simpleudf branch 2 times, most recently from 32780e0 to 3580343 Compare December 12, 2022 09:32

darrenfu commented Dec 12, 2022

View reviewed changes

darrenfu requested a review from mbasmanova December 12, 2022 09:36

darrenfu force-pushed the array_shuffle_simpleudf branch from 3580343 to 2ecc0c4 Compare December 12, 2022 17:39

darrenfu force-pushed the array_shuffle_simpleudf branch from 2ecc0c4 to b685a69 Compare December 12, 2022 17:47

darrenfu mentioned this pull request Dec 12, 2022

Support more types for Presto array UDF functions #3484

Open

darrenfu force-pushed the array_shuffle_simpleudf branch from 7428b07 to 814013b Compare December 12, 2022 19:04

darrenfu marked this pull request as draft December 16, 2022 07:34

darrenfu marked this pull request as ready for review December 22, 2022 03:13

darrenfu force-pushed the array_shuffle_simpleudf branch 2 times, most recently from 2166cc7 to b25d597 Compare December 22, 2022 03:17

darrenfu commented Dec 22, 2022

View reviewed changes

velox/functions/prestosql/tests/ArrayShuffleTest.cpp Outdated Show resolved Hide resolved

darrenfu marked this pull request as draft December 22, 2022 05:24

darrenfu force-pushed the array_shuffle_simpleudf branch from b25d597 to f689aa8 Compare December 22, 2022 05:27

darrenfu force-pushed the array_shuffle_simpleudf branch 2 times, most recently from 67da804 to 0ec77f4 Compare January 11, 2023 17:47

darrenfu requested a review from mbasmanova January 11, 2023 17:50

darrenfu force-pushed the array_shuffle_simpleudf branch from 49afc0d to a8e779a Compare January 12, 2023 22:03

mbasmanova reviewed Jan 18, 2023

View reviewed changes

darrenfu force-pushed the array_shuffle_simpleudf branch from 2fb6732 to 8996be8 Compare January 19, 2023 08:05

darrenfu requested a review from mbasmanova January 19, 2023 08:06

mbasmanova reviewed Jan 19, 2023

View reviewed changes

darrenfu force-pushed the array_shuffle_simpleudf branch from 178a69c to 015f69c Compare January 19, 2023 20:29

darrenfu requested a review from mbasmanova January 19, 2023 20:30

darrenfu force-pushed the array_shuffle_simpleudf branch from 015f69c to 56997cb Compare January 19, 2023 20:45

mbasmanova reviewed Jan 20, 2023

View reviewed changes

mbasmanova approved these changes Jan 20, 2023

View reviewed changes

darrenfu force-pushed the array_shuffle_simpleudf branch from b1aa69e to 3f7437c Compare January 23, 2023 18:51

darrenfu force-pushed the array_shuffle_simpleudf branch from 3f7437c to 5b75e78 Compare January 23, 2023 19:32

facebook-github-bot closed this in 64f090d Jan 25, 2023

facebook-github-bot added the Merged label Jan 25, 2023

darrenfu mentioned this pull request Jan 26, 2023

[native] Add an e2e test for shuffle function prestodb/presto#18983

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shuffle Presto function #3404

Add shuffle Presto function #3404

darrenfu commented Dec 2, 2022 •

edited

Loading

netlify bot commented Dec 2, 2022 •

edited

Loading

mbasmanova left a comment

darrenfu left a comment

mbasmanova commented Dec 12, 2022

darrenfu commented Dec 12, 2022

darrenfu commented Dec 12, 2022

darrenfu commented Jan 11, 2023 •

edited

Loading

mbasmanova commented Jan 11, 2023

darrenfu commented Jan 11, 2023 •

edited

Loading

mbasmanova left a comment

mbasmanova Jan 18, 2023

mbasmanova Jan 18, 2023

darrenfu Jan 18, 2023

mbasmanova Jan 18, 2023

darrenfu commented Jan 19, 2023 •

edited

Loading

mbasmanova left a comment

mbasmanova Jan 19, 2023

darrenfu Jan 19, 2023

mbasmanova Jan 19, 2023

darrenfu Jan 19, 2023

mbasmanova Jan 19, 2023

darrenfu Jan 19, 2023 •

edited

Loading

darrenfu commented Jan 20, 2023 •

edited

Loading

mbasmanova left a comment

facebook-github-bot commented Jan 20, 2023

facebook-github-bot commented Jan 23, 2023

facebook-github-bot commented Jan 23, 2023

facebook-github-bot commented Jan 25, 2023

Add shuffle Presto function #3404

Add shuffle Presto function #3404

Conversation

darrenfu commented Dec 2, 2022 • edited Loading

netlify bot commented Dec 2, 2022 • edited Loading

✅ Deploy Preview for meta-velox canceled.

mbasmanova left a comment

Choose a reason for hiding this comment

darrenfu left a comment

Choose a reason for hiding this comment

mbasmanova commented Dec 12, 2022

darrenfu commented Dec 12, 2022

darrenfu commented Dec 12, 2022

darrenfu commented Jan 11, 2023 • edited Loading

mbasmanova commented Jan 11, 2023

darrenfu commented Jan 11, 2023 • edited Loading

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darrenfu commented Jan 19, 2023 • edited Loading

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darrenfu Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

darrenfu commented Jan 20, 2023 • edited Loading

mbasmanova left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 20, 2023

facebook-github-bot commented Jan 23, 2023

facebook-github-bot commented Jan 23, 2023

facebook-github-bot commented Jan 25, 2023

darrenfu commented Dec 2, 2022 •

edited

Loading

netlify bot commented Dec 2, 2022 •

edited

Loading

darrenfu commented Jan 11, 2023 •

edited

Loading

darrenfu commented Jan 11, 2023 •

edited

Loading

darrenfu commented Jan 19, 2023 •

edited

Loading

darrenfu Jan 19, 2023 •

edited

Loading

darrenfu commented Jan 20, 2023 •

edited

Loading