Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.A very small patch, but be careful of the unit test. It bites. :-) See the docblock. I can also see us leaving out the unit test for this one since it logically cannot be perfect.
Note that the implementation here is NOT optimal. It's not supposed to be the most performant, just the most stable. We can look into DB-specific optimizations post-freeze, as those won't require an API change.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | test_select_random.patch | 5.17 KB | David_Rothstein |
| #8 | select_random.patch | 4.95 KB | Crell |
| #7 | select_random.patch | 4.81 KB | Crell |
| #4 | select_random.patch | 4.53 KB | Crell |
| #2 | select_random.patch | 4.53 KB | Crell |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe logic looks sound, but there are lot of coding style issues in there.
Comment #2
Crell CreditAttribution: Crell commentedLots? I found only one tab. You'll have to be more specific because I don't see any others.
Comment #3
Crell CreditAttribution: Crell commentedTagging.
Comment #4
Crell CreditAttribution: Crell commentedMinor comment and whitespace updates per review from DamZ.
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedYay.
Comment #6
chx CreditAttribution: chx commentedThis can not get into core like this, we need to add a big red ugly warning that this does not scale to big resultsets. http://jan.kneschke.de/projects/mysql/order-by-rand/ however the scalable version will be very MySQL specific and thus is contrib. And that's fine. But I want my warning :)
Comment #7
Crell CreditAttribution: Crell commentedAsk and ye shall receive. No code changes so back to RTBC.
Comment #8
Crell CreditAttribution: Crell commentedSlightly improved comment.
Comment #9
chx CreditAttribution: chx commentedI am OK with this.
Comment #10
webchickHm. I'm a little nervous about this test. We'll have to see if it starts affecting testbot with other patches in the queue.
Otherwise this looks well-documented, and adds in a missing feature from DBTNG. Committed to HEAD.
This needs to be documented in the upgrade docs.
Comment #11
David_Rothstein CreditAttribution: David_Rothstein commentedEh, with my luck, all my patches are going to be in the 1% that randomly fails :) Also, if I'm looking at the numbers right, I think the probability of this test failing is actually higher than 1%...
I'd like to suggest that perhaps we aren't even testing the correct thing here. We need to test the Drupal database layer, but this test is essentially also trying to assert that MySQL has a good random number generator. I don't think we need or want to go down that road (the test is barely scratching the surface of that, anyway).
The attached patch takes a different approach. It basically just asserts that when you run the query twice, you get the same results in a different order. If the same exact select query gives a differently ordered result each time, it's hard to imagine any possible way that could happen without having triggered the database's randomization process.
Furthermore, this also tests that the random ordering is actually an "ordering" (i.e., that there are no repeats), which the previous test did not do.
The chance of this test failing by accident is effectively zero, so I'd consider it a big improvement :)
Comment #12
Crell CreditAttribution: Crell commentedYeah, I do like the test in #11 better.
Comment #13
Crell CreditAttribution: Crell commentedDocumentation has been added, too, and this is no longer an API change just test improvement.
Comment #14
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #15
chx CreditAttribution: chx commented$this->assertNotEqual($randomized_ids, $ordered_ids, t('A query with random ordering returns an unordered set of IDs.'));
Huh? Why would a randomization be always different from the ordering-by-id result? If it's random then it can be anything. I agree the chances of two randomizations being the same are sufficiently low, however it can happen theoretically (if it's independent and random...)
Comment #16
Crell CreditAttribution: Crell commentedThe previous test was not deterministically perfect either. The new one is, arguably, simpler. The odds of it returning a false negative are easy to calculate, and really really low. (1 in 52!, I believe). I don't know off hand what the false negative odds were on the previous test.
Comment #17
David_Rothstein CreditAttribution: David_Rothstein commentedYeah, to put that number in perspective, the Sun is going to burn out long, long, long before this test ever randomly fails. I think we're OK :)
I don't remember the exact number, but the odds of the previous version failing were something around 1%, which was one of the motivations for changing it. 1% is pretty high - it would hit us all the time.
Also, don't forget that we call $this->randomName() hundreds of times in the tests. That could cause failures too, if it stumbled upon something that caused a conflict. That one actually might happen before the Sun burns out... although not much before :)
Comment #18
Crell CreditAttribution: Crell commentedNo, we could have a false negative later today. The odds of that are 1 in 52!. We could also have a false negative tomorrow. The odds of that are also 1 in 52!. The odds of both happening are 1 in 52!*52!. Miniscule, but still possible.
Comment #19
David_Rothstein CreditAttribution: David_Rothstein commentedLet's put it this way - I'm willing to bet a very large sum of money that the Sun will burn out first ... Of course, it would be difficult to collect on that bet :)