fix(*): Method.Random[Key] behavior
#269
Conversation
|
Looks good other than the issues above. Will have to make sure the tests pass. |
Codecov Report
@@ Coverage Diff @@
## main #269 +/- ##
==========================================
+ Coverage 88.11% 93.52% +5.41%
==========================================
Files 3 2 -1
Lines 1228 1019 -209
Branches 218 211 -7
==========================================
- Hits 1082 953 -129
+ Misses 80 39 -41
+ Partials 66 27 -39
|
Except mongo dan fix your stuff
WilsontheWolf
force-pushed
the
fix/random-behaviour
branch
from
9f04fce to
68f2e4b
Compare
| payload.data.push(this.cache.get(key)!); | ||
| } | ||
| } else { | ||
| if (unique) { | ||
| const randomKeys = new Set<string>(); | ||
|
|
||
| while (randomKeys.size < count) randomKeys.add(keys[Math.floor(Math.random() * keys.length)]); | ||
|
|
||
| for (const key of randomKeys) payload.data.push(this.cache.get(key)!); |
There was a problem hiding this comment.
Is there a way to not need 2 loops here?
There was a problem hiding this comment.
You could probably just payload.data = randomKeys
There was a problem hiding this comment.
We have to get the value with the keys, so yes.
There was a problem hiding this comment.
I guess we could probably get the key and fetch the value in the same loop though
| payload.data.push((await this.handler.get(key))!); | ||
| } | ||
| } else { | ||
| if (unique) { | ||
| const randomKeys = new Set<string>(); |
There was a problem hiding this comment.
Does mariadb have any way to query random that would be faster than this?
There was a problem hiding this comment.
Looks like it does have some kind of random query but its somehow worse than ours
https://mariadb.com/kb/en/rand/
Note, however, that this technique should never be used on a large table as it will be extremely slow. MariaDB will read all rows in the table, generate a random value for each of them, order them, and finally will apply the LIMIT clause.
There was a problem hiding this comment.
Can we test this, it's possible while not the best it's better than the current solution
There was a problem hiding this comment.
I mean you can. My Maria doesn't work.
However just based off that description, it sounds to me like that is slower. It has to fetch all the keys, generate a random value for each key in the table, sort them and then choose the top x
Whereas ours is fetch all the keys, and repeat the amount of times we need to get, picking a random index
| payload.data.push((await this.handler.get(key))!); | ||
| } | ||
| } else { | ||
| if (unique) { | ||
| const randomKeys = new Set<string>(); | ||
|
|
||
| while (randomKeys.size < count) randomKeys.add(keys[Math.floor(Math.random() * keys.length)]); |
There was a problem hiding this comment.
Same as Maria, any cool database things we can do to make this more optimized?
There was a problem hiding this comment.
Seems like the same as maria, where it can generate a random number, but not efficently get random keys.
https://www.techonthenet.com/postgresql/functions/random.php
| if (unique) { | ||
| const randomKeys = new Set<string>(); | ||
|
|
||
| while (randomKeys.size < docCount) randomKeys.add(keys.data[Math.floor(Math.random() * keys.data.length)]); |
There was a problem hiding this comment.
I'm very sure redis has a random way of getting documents
There was a problem hiding this comment.
There was a problem hiding this comment.
nvm this appears to be for a set stored in redis. There is randomKey but it just gets one so its probably just as simple to do this (https://redis.io/commands/randomkey/)
There was a problem hiding this comment.
@dan-online can you do this, this is your provider after all.
| payload.data.push(this.handler.get(key)!); | ||
| } | ||
| } else { | ||
| if (unique) { | ||
| const randomKeys = new Set<string>(); | ||
|
|
||
| while (randomKeys.size < count) randomKeys.add(keys[Math.floor(Math.random() * keys.length)]); |
There was a problem hiding this comment.
Seems the same as the other 2 sql languages
blocked by josh-development/utilities#226