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

fix: zscore return 0 when double value is too small #344

Merged
merged 3 commits into from
Oct 4, 2023

Conversation

zsh1995
Copy link
Contributor

@zsh1995 zsh1995 commented Sep 20, 2023

I use miniredis to zadd 0.000000000000000000000000000000000000000000000000000000000000000025339988685347402, then zscore command return a 0, while using real redis server can correctly execute the zadd and zscore 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.

@alicebob
Copy link
Owner

Hi! Thanks for the PR. That was already painful to get working back then :(
Currently the tests fail with the normal floating point problems...

@zsh1995
Copy link
Contributor Author

zsh1995 commented Sep 21, 2023

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

@alicebob
Copy link
Owner

alicebob commented Sep 21, 2023 via email

@alicebob
Copy link
Owner

I'm getting different results, with redis 7.2 on an AMD64 machine:

~/src/miniredis/integration/redis_src$ ./redis-cli 
127.0.0.1:6379> zadd a_zset 1.2 one
(integer) 0
127.0.0.1:6379> zadd a_zset incr 1.2 one
"2.4"
127.0.0.1:6379> zadd a_zset incr 1.2 one
"3.5999999999999996"
127.0.0.1:6379> zadd a_zset incr 1.2 one
"4.8"
127.0.0.1:6379> 

@alicebob
Copy link
Owner

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).

@alicebob
Copy link
Owner

alicebob commented Oct 1, 2023

short of implementing grisu2 in Go

I did just that (#347), but it needs a bit more work still.

@alicebob alicebob merged commit aa376e7 into alicebob:master Oct 4, 2023
4 checks passed
@alicebob
Copy link
Owner

alicebob commented Oct 4, 2023

Thanks, merged! I'll merge the grisu pr over this as well, so the formatting in real and mini redis should be identical.

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.

None yet

2 participants