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.

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

implement array_shuffle #4220

Merged
merged 2 commits into from
Sep 6, 2024
Merged

Conversation

gushul
Copy link
Contributor

@gushul gushul commented Aug 31, 2024

Implemented array_shuffle function for postgres under #4153

@weiznich weiznich requested a review from a team August 31, 2024 07:48
@weiznich
Copy link
Member

Seems like the CI job fails as we use a Postgres version for our tests that does not support array_shuffle yet. I will look at how to solve that next week.

/// let shuffled = diesel::select(array_shuffle::<Array<Integer>, _>(vec![1, 2, 3, 4, 5]))
/// .get_result::<Vec<i32>>(connection)?;
/// assert_eq!(5, shuffled.len());
/// assert!(shuffled != vec![1, 2, 3, 4, 5]); // It's very unlikely to get the same order
Copy link

Choose a reason for hiding this comment

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

Obviously this test will fail in just under 1% of runs, when the shuffle happens to do nothing. I'm a bit uncomfortable about this - one test like this might be fine, but if we set a precedent and we had many tests like this our suite will become flaky and "just rerun the suite" will become a common approach rather than digging into the problem.
@weiznich please let me know if you think I'm being overcautious.
A simple alternative would be

  println!("{:?}", shuffled); // e.g., `[3, 2, 5, 1, 4]`

Copy link
Contributor

Choose a reason for hiding this comment

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

You can set bits corresponding to numbers in vec and check that the result is 31.

Copy link

Choose a reason for hiding this comment

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

Yep, or sort the result in Rust and compare with the sorted array.

@weiznich
Copy link
Member

weiznich commented Sep 3, 2024

I had another look at this and I think the best way forward for now is to mark this test as no_run doc-test as we cannot expect that all contributors have postgres 16 installed locally. That would mean we compile but don't run the test, which is fine for this case. We also should have a comment there saying that the no_run setting is there because the test requires postgres 16.

@weiznich weiznich marked this pull request as ready for review September 6, 2024 06:26
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

I've just changed the test to no_run, otherwise this looks good to me. Thanks for working on this 👍

@weiznich weiznich added this pull request to the merge queue Sep 6, 2024
Merged via the queue into diesel-rs:master with commit bc0e030 Sep 6, 2024
48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants