-
Notifications
You must be signed in to change notification settings - Fork 209
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
fix: zscore return 0 when double value is too small #344
Conversation
Hi! Thanks for the PR. That was already painful to get working back then :( |
Thanks Reply! I try this on redis:
It seems that if we want to be consistent with redis, we need to adjust the test cases |
On Wed, Sep 20, 2023 at 08:13:32PM -0700, zsh1995 wrote:
Thanks Reply! I try this on redis:
```
127.0.0.1:7079> zadd a_zset 1.2 one
(integer) 0
127.0.0.1:7079> zadd a_zset incr 1.2 one
"2.3999999999999999"
127.0.0.1:7079> zadd a_zset incr 1.2 one
"3.5999999999999996"
127.0.0.1:7079> zadd a_zset incr 1.2 one
"4.7999999999999998"
```
It seems that if we want to be consistent with redis, we need to adjust the test cases
Oh, that's interesting. I'll have a proper look next week.
|
I'm getting different results, with redis 7.2 on an AMD64 machine:
|
Redis uses "grisu2" from pdf linked in https://florian.loitsch.com/publications --> https://drive.google.com/file/d/0BwvYOx00EwKmcTZUM0Q4VW1DTHc/view?resourcekey=0-gW-moqRGyWpqFsyhiBI_-w Go does not do exactly the same thing, but short of implementing grisu2 in Go (sounds fun!), I think this is Good Enough. You were absolutely right in that it should use the "g" option to convert the float (meaning: switch to scientific if needed). |
I did just that (#347), but it needs a bit more work still. |
Thanks, merged! I'll merge the grisu pr over this as well, so the formatting in real and mini redis should be identical. |
I use miniredis to
zadd
0.000000000000000000000000000000000000000000000000000000000000000025339988685347402
, thenzscore
command return a 0, while using real redis server can correctly execute thezadd
andzscore
command.I found this issuse: #17 already trying to fix this problem, but didn't pass integration test.
I read redis's source code https://github.com/redis/redis/blob/unstable/deps/fpconv/fpconv_dtoa.c#L349 , I believe my approach is basically consistent with it.